[initial-letter] Implement parsing CSS property initail-letter [chromium/src : main]

Yoshifumi Inoue (Gerrit)

unread,

Oct 11, 2022, 3:53:05 AM10/11/22

to Kent Tamura, Koji Ishii, apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org

Attention is currently required from: Kent Tamura, Koji Ishii.

Yoshifumi Inoue would like Kent Tamura and Koji Ishii to review this change.

View Change

[initial-letter] Implement parsing CSS property initail-letter

This patch introduces parsing CSS property "initail-letter". Following
CLs will implement layout and paint[1].

[1] http://crrev.com/c/3874383: [initial-letter] all in one

Bug: 1276900
Change-Id: Id0b7dedd30a62ce4658262b6f7c86424ce18766d
---
M third_party/blink/public/mojom/use_counter/metrics/css_property_id.mojom
M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/css_value_keywords.json5
M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.h
M third_party/blink/renderer/core/style/build.gni
A third_party/blink/renderer/core/style/style_initial_letter.cc
A third_party/blink/renderer/core/style/style_initial_letter.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
A third_party/blink/web_tests/external/wpt/css/css-initial-letter/initial-letter-valid.html
14 files changed, 276 insertions(+), 0 deletions(-)

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Oct 11, 2022, 3:55:03 AM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Kent Tamura, Koji Ishii, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Kent Tamura, Koji Ishii.

Patch set 2:Auto-Submit +1Commit-Queue +1

View Change

1 comment:

Patchset:
- Patch Set #2:
  PTAL

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Blink W3C Test Autoroller (Gerrit)

unread,

Oct 11, 2022, 4:01:49 AM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Kent Tamura, Koji Ishii, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Kent Tamura, Koji Ishii.

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/36395.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

View Change

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Kent Tamura (Gerrit)

unread,

Oct 11, 2022, 4:02:41 AM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Blink W3C Test Autoroller, Kent Tamura, Koji Ishii, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Koji Ishii, Yoshifumi Inoue.

View Change

3 comments:

Patchset:
- Patch Set #2:
  I don't have comments other than the followings. Please ask the style team for review.
File third_party/blink/renderer/core/style/style_initial_letter.h:
- Patch Set #2, Line 1: // Copyright 2022 The Chromium Authors. All rights reserved.
  This is the old copyright header. Please follow https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#file-headers
File third_party/blink/renderer/core/style/style_initial_letter.cc:
- Patch Set #2, Line 1: // Copyright 2022 The Chromium Authors. All rights reserved.
  This is the old copyright header. Please follow https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#file-headers

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Koji Ishii (Gerrit)

unread,

Oct 11, 2022, 4:09:02 AM10/11/22

to Rune Lillesveen, apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Kent Tamura

Attention is currently required from: Rune Lillesveen, Yoshifumi Inoue.

Koji Ishii would like Rune Lillesveen to review this change authored by Yoshifumi Inoue.

View Change

[initial-letter] Implement parsing CSS property initail-letter

This patch introduces parsing CSS property "initail-letter". Following
CLs will implement layout and paint[1].

[1] http://crrev.com/c/3874383: [initial-letter] all in one

Bug: 1276900
Change-Id: Id0b7dedd30a62ce4658262b6f7c86424ce18766d
---
M third_party/blink/public/mojom/use_counter/metrics/css_property_id.mojom
M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/css_value_keywords.json5
M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.h
M third_party/blink/renderer/core/style/build.gni
A third_party/blink/renderer/core/style/style_initial_letter.cc
A third_party/blink/renderer/core/style/style_initial_letter.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
A third_party/blink/web_tests/external/wpt/css/css-initial-letter/initial-letter-valid.html
14 files changed, 276 insertions(+), 0 deletions(-)

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Koji Ishii (Gerrit)

unread,

Oct 11, 2022, 4:10:44 AM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Rune Lillesveen, Blink W3C Test Autoroller, Kent Tamura, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Rune Lillesveen, Yoshifumi Inoue.

View Change

1 comment:

Patchset:
- Patch Set #2:
  +Rune

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Koji Ishii (Gerrit)

unread,

Oct 11, 2022, 4:46:48 AM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Rune Lillesveen, Blink W3C Test Autoroller, Kent Tamura, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Rune Lillesveen, Yoshifumi Inoue.

View Change

2 comments:

File third_party/blink/renderer/core/style/style_initial_letter.h:
- Patch Set #2, Line 32:
```
  bool IsDrop() const { return sink_type_ == kDrop; }
  bool IsNormal() const { return !size_; }
  bool IsRaise() const { return sink_type_ == kRaise; }
```
  Wouldn't `IsDrop` and `IsRaise` return a wrong value if `HasSink()`?
- Patch Set #2, Line 36: static_cast<int>(size_)
  From the spec, this should be `1` if `raise`, or the floored number of size if `drop`.
  https://drafts.csswg.org/css-inline/#valdef-initial-letter-raise

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Koji Ishii (Gerrit)

unread,

Oct 11, 2022, 1:01:08 PM10/11/22

to Anders Hartvoll Ruud, apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Rune Lillesveen, Kent Tamura

Attention is currently required from: Anders Hartvoll Ruud, Rune Lillesveen, Yoshifumi Inoue.

Koji Ishii would like Anders Hartvoll Ruud to review this change authored by Yoshifumi Inoue.

View Change

[initial-letter] Implement parsing CSS property initail-letter

This patch introduces parsing CSS property "initail-letter". Following
CLs will implement layout and paint[1].

[1] http://crrev.com/c/3874383: [initial-letter] all in one

Bug: 1276900
Change-Id: Id0b7dedd30a62ce4658262b6f7c86424ce18766d
---
M third_party/blink/public/mojom/use_counter/metrics/css_property_id.mojom
M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/css_value_keywords.json5
M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.h
M third_party/blink/renderer/core/style/build.gni
A third_party/blink/renderer/core/style/style_initial_letter.cc
A third_party/blink/renderer/core/style/style_initial_letter.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
A third_party/blink/web_tests/external/wpt/css/css-initial-letter/initial-letter-valid.html
14 files changed, 276 insertions(+), 0 deletions(-)

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Koji Ishii (Gerrit)

unread,

Oct 11, 2022, 1:03:21 PM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Anders Hartvoll Ruud, Rune Lillesveen, Blink W3C Test Autoroller, Kent Tamura, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Anders Hartvoll Ruud, Rune Lillesveen, Yoshifumi Inoue.

View Change

1 comment:

Patchset:
- Patch Set #2:
  +andruud given the style-dev discussion.

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Anders Hartvoll Ruud (Gerrit)

unread,

Oct 11, 2022, 7:43:32 PM10/11/22

to apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, Rune Lillesveen, Blink W3C Test Autoroller, Kent Tamura, Koji Ishii, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org

Attention is currently required from: Rune Lillesveen, Yoshifumi Inoue.

View Change

10 comments:

Commit Message:
- Patch Set #2, Line 7: initail
  initial?
- Patch Set #2, Line 9: initail
  initial?
File third_party/blink/renderer/core/css/properties/css_parsing_utils.cc:
- Patch Set #2, Line 5652: ConsumeInteger
  IIRC this would not allow `initial-letter: 1 calc(1.1)`. (Math functions are allowed to bypass the <integer> restriction, and are instead clamped computed-value time). Try `ConsumeIntegerOrNumberCalc` instead.
  (Eventually ConsumeInteger should just become that function, but we are not there yet ...)
  See the following for more information: https://w3c.github.io/csswg-drafts/css-values-4/#calc-range:~:text=rounded%20to%20the%20nearest%20integer%2C
File third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc:
- Patch Set #2, Line 3687:
```
if (initial_letter.IsNormal())
    return CSSIdentifierValue::Create(CSSValueID::kNormal);
```
  In that case, maybe it's more consistent to also return a `CSSIdentifierValue::Create(CSSValueID::kNormal)` directly from `ConsumeInitialLetter` rather than wrapping it in a list.
- Patch Set #2, Line 3694: kNumber
  kInteger?
File third_party/blink/renderer/core/css/resolver/style_builder_converter.cc:
- Patch Set #2, Line 1413: const auto& list = To<CSSValueList>(value);
  This function does too much fallback to `StyleInitialLetter()` if something unexpected happens, which isn't really right. Generally the parsing function must emit something compatible, and we should DCHECK if something unexpected is observed.
  I'd get the `normal` case out of the way first, then do something like:
```
DCHECK(list->length() == 1 || list->length() == 2);
CSSValue* first = list->Item(0);
CSSValue* second = list->length() == 2 ? list->Item(1) : nullptr;
// std::swap items if `first` contains drop/raise ident, then take it from there.
```
- Patch Set #2, Line 1446: size->GetFloatValue() >= 1
  We should DCHECK this, it should not be possible to be out-of-range here.
- Patch Set #2, Line 1450:
```
size->GetFloatValue() >= 1) {
    if (sink && sink->GetIntValue() >= 1
```
  Same with these range checks.
File third_party/blink/web_tests/external/wpt/css/css-initial-letter/initial-letter-valid.html:
- Patch Set #2, Line 1: <!DOCTYPE html>
  You probably also want to get coverage on _computed values_, see e.g. aspect-ratio-computed.html for inspiration.
- Patch Set #2, Line 21: test_valid_value('initial-letter', '1.23 456');
  Recommend e.g. `test_valid_value('initial-letter', '1.23 calc(45.1)');` as well.

To view, visit change 3944351. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward