[list-style-position-quirk] Change `<li>` to match the spec [chromium/src : main]
Koji Ishii (Gerrit)
Koji Ishii voted and added 2 comments![]( "Open in Gerrit")
Votes added by Koji Ishii
| Commit-Queue | +1 |
2 comments
/* The `:is()` hits `DCHECK(default_html_quirks_style_->UniversalRules().empty())` */I'm not sure why `:is()` in `quirks.css` causes `DCHECK(default_html_quirks_style_->UniversalRules().empty())` in `VerifyUniversalRuleCount()`. `:is()` isn't a universal selector, is it? Also, `html.css` has `:is()` without problems. Can you advise?
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 4 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
4 comments
This patch changes to match the WebKit. In particular:```suggestion
This patch changes to match the WebKit behavior. In particular:
```
* Implements the styles for quirks mode in the [HTML spec][1]
to mitigate the risk of web-compat. ~0.8%[3] still hits the
code path.And despite this, you're comfortable doing this without running it by API_OWNERS since it's aligning with both others browsers, I assume.
/* The `:is()` hits `DCHECK(default_html_quirks_style_->UniversalRules().empty())` */I'm not sure why `:is()` in `quirks.css` causes `DCHECK(default_html_quirks_style_->UniversalRules().empty())` in `VerifyUniversalRuleCount()`. `:is()` isn't a universal selector, is it? Also, `html.css` has `:is()` without problems. Can you advise?
Yeah, the `UniversalRules` _bucket_ contains more than just universal rules; it contains any rule that we were not able to place in a more narrow bucket. For example, the selector `:not(.a)` isn't strictly _universal_, but it still goes in the universal bucket, since we have no concept of bucketing for "consider everything except .a".
`:is(dir, menu, ol, ul)` can not be placed in a more narrow bucket, because we need to place it in _exactly one_ of the [dir, menu, ol, ul] buckets; we don't yet have a concept of "multi-bucketing", though sesse@ has been considering something like that lately.
Also, html.css has :is() without problems.
That's because it's using `:is()` in a non-subject position; only the rightmost compound selector matters for bucketing. This means that you should able to refactor your last selector list behemoth into the following without hitting a DCHECK:
```
:is(dir, menu, ol, ul) dir,
:is(dir, menu, ol, ul) menu,
:is(dir, menu, ol, ul) ol,
:is(dir, menu, ol, ul) ul,
:is(dir, menu, ol, ul) li
```
I'll leave it up to you whether or not you think that's an improvement. :-)
name: "ListStylePositionQuirk",The name was a bit confusing at first. If I understand this correctly, disabling this flag doesn't remove the quirk (from quirks mode), but instead applies the quirky behavior (the old behavior) to standards mode _as well_.
I would at least comment that this doesn't just toggle behavior in quirks mode, but also affects whether or not we follow the standard in regular mode.
Related details
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 5 comments![]( "Open in Gerrit")
Thank you for the prompt review as always.
This patch changes to match the WebKit. In particular:```suggestion
This patch changes to match the WebKit behavior. In particular:
```
Done
* Implements the styles for quirks mode in the [HTML spec][1]
to mitigate the risk of web-compat. ~0.8%[3] still hits the
code path.And despite this, you're comfortable doing this without running it by API_OWNERS since it's aligning with both others browsers, I assume.
Yes, I believe this is fine, this is an interop-2025 item.
/* The `:is()` hits `DCHECK(default_html_quirks_style_->UniversalRules().empty())` */Anders Hartvoll RuudI'm not sure why `:is()` in `quirks.css` causes `DCHECK(default_html_quirks_style_->UniversalRules().empty())` in `VerifyUniversalRuleCount()`. `:is()` isn't a universal selector, is it? Also, `html.css` has `:is()` without problems. Can you advise?
Yeah, the `UniversalRules` _bucket_ contains more than just universal rules; it contains any rule that we were not able to place in a more narrow bucket. For example, the selector `:not(.a)` isn't strictly _universal_, but it still goes in the universal bucket, since we have no concept of bucketing for "consider everything except .a".
`:is(dir, menu, ol, ul)` can not be placed in a more narrow bucket, because we need to place it in _exactly one_ of the [dir, menu, ol, ul] buckets; we don't yet have a concept of "multi-bucketing", though sesse@ has been considering something like that lately.
Also, html.css has :is() without problems.
That's because it's using `:is()` in a non-subject position; only the rightmost compound selector matters for bucketing. This means that you should able to refactor your last selector list behemoth into the following without hitting a DCHECK:
```
:is(dir, menu, ol, ul) dir,
:is(dir, menu, ol, ul) menu,
:is(dir, menu, ol, ul) ol,
:is(dir, menu, ol, ul) ul,
:is(dir, menu, ol, ul) li
```I'll leave it up to you whether or not you think that's an improvement. :-)
Understand, thank you for the explanation! I'll take your suggestion as it's easier to find errors for me.
The name was a bit confusing at first. If I understand this correctly, disabling this flag doesn't remove the quirk (from quirks mode), but instead applies the quirky behavior (the old behavior) to standards mode _as well_.
I would at least comment that this doesn't just toggle behavior in quirks mode, but also affects whether or not we follow the standard in regular mode.
Appended "Standard", and also added comment. Hope it's clearer.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/css/resolver/style_adjuster.cc
Insertions: 1, Deletions: 1.
@@ -610,7 +610,7 @@
return;
}
- if (!RuntimeEnabledFeatures::ListStylePositionQuirkEnabled()) {
+ if (!RuntimeEnabledFeatures::ListStylePositionQuirkStandardEnabled()) {
if (IsA<HTMLUListElement>(element) || IsA<HTMLOListElement>(element)) {
builder.SetIsInsideListElement();
return;
```
```
The name of the file: third_party/blink/renderer/core/html/resources/quirks.css
Insertions: 8, Deletions: 6.
@@ -59,19 +59,21 @@
margin-block-end: 1em
}
-@supports blink-feature(ListStylePositionQuirk) {
+@supports blink-feature(ListStylePositionQuirkStandard) {
/* https://html.spec.whatwg.org/multipage/rendering.html#lists */
li { list-style-position: inside; }
- /* The `:is()` hits `DCHECK(default_html_quirks_style_->UniversalRules().empty())` */
+ /* Expand `:is()` in the rightmost position because otherwise they hit
+ `DCHECK(default_html_quirks_style_->UniversalRules().empty())` */
/* li :is(dir, menu, ol, ul) { list-style-position: outside; }*/
li dir, li menu, li ol, li ul { list-style-position: outside; }
/* :is(dir, menu, ol, ul) :is(dir, menu, ol, ul, li) { */
- dir dir, dir menu, dir ol, dir ul, dir li,
- menu dir, menu menu, menu ol, menu ul, menu li,
- ol dir, ol menu, ol ol, ol ul, ol li,
- ul dir, ul menu, ul ol, ul ul, ul li {
+ :is(dir, menu, ol, ul) dir,
+ :is(dir, menu, ol, ul) menu,
+ :is(dir, menu, ol, ul) ol,
+ :is(dir, menu, ol, ul) ul,
+ :is(dir, menu, ol, ul) li {
list-style-position: unset;
}
}
```
```
The name of the file: third_party/blink/renderer/core/style/computed_style_test.cc
Insertions: 1, Deletions: 1.
@@ -2148,7 +2148,7 @@
}
TEST_F(ComputedStyleTest, UseCountInsideListMarkerPositionQuirk) {
- if (RuntimeEnabledFeatures::ListStylePositionQuirkEnabled()) {
+ if (RuntimeEnabledFeatures::ListStylePositionQuirkStandardEnabled()) {
return;
}
Document& document = GetDocument();
```
```
The name of the file: third_party/blink/renderer/core/style/computed_style.cc
Insertions: 1, Deletions: 1.
@@ -2775,7 +2775,7 @@
ListStylePosition() == EListStylePosition::kInside) {
return true;
}
- if (!RuntimeEnabledFeatures::ListStylePositionQuirkEnabled()) {
+ if (!RuntimeEnabledFeatures::ListStylePositionQuirkStandardEnabled()) {
// Force the marker of <li> elements with no <ol> or <ul> ancestor to have
// an inside position.
// TODO(crbug.com/41241289): This quirk predates WebKit, it was added to
```
```
The name of the file: third_party/blink/renderer/platform/runtime_enabled_features.json5
Insertions: 3, Deletions: 1.
@@ -1578,7 +1578,7 @@
{
// crbug.com/1463890: CSS `text-autospace` property
name: "CSSTextAutoSpace",
- status: "experimental",
+ status: "stable",
},
{
// crbug.com/1463890, crbug.com/1463891: CSS `text-spacing` shorthand
@@ -2861,7 +2861,9 @@
status: "stable",
},
{
- name: "ListStylePositionQuirk",
+ // Enabling this changes the `list-style-position` quirk to match the
+ // HTML standard spec.
+ name: "ListStylePositionQuirkStandard",
status: "stable",
},
{
```
Change information
[list-style-position-quirk] Change `<li>` to match the spec
This patch changes the default styling of `<li>` elements when
they are not parented by `<ol>` or `<ul>`.
The behavior before this patch was inherited from WebKit,
which copied the behavior from Internet Explorer 7. Since
then, the [HTML spec][1] was updated, and Gecko and WebKit[2]
matched the spec.
This patch changes to match the WebKit behavior. In particular:
* `<li>` not parented by `<ol>` or `<ul>` uses the same used
value of `list-style-position` as when parented by `<ol>` or
`<ul>`.
* Implements the styles for quirks mode in the [HTML spec][1]
to mitigate the risk of web-compat. ~0.8%[3] still hits the
code path.
The changes are under a runtime flag
`ListStylePositionQuirkStandard`.
[1]: https://html.spec.whatwg.org/multipage/rendering.html#lists
[2]: https://github.com/WebKit/WebKit/commit/3be0078f2601ceca057ebd096ae3384d75cdd8b4
[3]: https://chromestatus.com/metrics/feature/timeline/popularity/5000
- M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
- M third_party/blink/renderer/core/html/resources/quirks.css
- M third_party/blink/renderer/core/layout/inline/inline_node_test.cc
- M third_party/blink/renderer/core/layout/layout_text_combine_test.cc
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/renderer/core/style/computed_style_extra_fields.json5
- M third_party/blink/renderer/core/style/computed_style_test.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/TestExpectations
- A third_party/blink/web_tests/external/wpt/html/rendering/non-replaced-elements/lists/list-style-position-quirks-mode-ref.html
- A third_party/blink/web_tests/external/wpt/html/rendering/non-replaced-elements/lists/list-style-position-quirks-mode.html
- M third_party/blink/web_tests/external/wpt/html/rendering/non-replaced-elements/lists/lists-styles-quirks-expected.txt
- M third_party/blink/web_tests/fast/lists/inline-before-content-after-list-marker.html
- M third_party/blink/web_tests/fast/lists/list-marker-computed-display.html
Code-Review: +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53596
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oriol Brufau (Gerrit)
Oriol Brufau added 2 comments![]( "Open in Gerrit")
* Implements the styles for quirks mode in the [HTML spec][1]
to mitigate the risk of web-compat. ~0.8%[3] still hits the
code path.Koji IshiiAnd despite this, you're comfortable doing this without running it by API_OWNERS since it's aligning with both others browsers, I assume.
Yes, I believe this is fine, this is an interop-2025 item.
Note that when I added the counter, I didn't think about restricting it to non-quirks mode. Pages in quirks mode will probably not be affected because of the new CSS in quirks.css. So the relevant percentage might be lower.
<!-- <li> elements with no <ol> or <ul> ancestor are forced to have inside markers -->This comment is no longer true.
Related details
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii added 2 comments![]( "Open in Gerrit")
* Implements the styles for quirks mode in the [HTML spec][1]
to mitigate the risk of web-compat. ~0.8%[3] still hits the
code path.Koji IshiiAnd despite this, you're comfortable doing this without running it by API_OWNERS since it's aligning with both others browsers, I assume.
Oriol BrufauYes, I believe this is fine, this is an interop-2025 item.
Note that when I added the counter, I didn't think about restricting it to non-quirks mode. Pages in quirks mode will probably not be affected because of the new CSS in quirks.css. So the relevant percentage might be lower.
Thanks for your comment. I think I'm going to make the counter obsolete as we're no longer counting them when the runtime flag is on. Please let me know if you think otherwise.
<!-- <li> elements with no <ol> or <ul> ancestor are forced to have inside markers -->This comment is no longer true.
Thank you Oriol, removing at https://chromium-review.googlesource.com/c/chromium/src/+/6712076
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |