Fix WPT CSS Counters test #41799
Merged
Fix WPT CSS Counters test #41799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Sep 4, 2023
Loirooriol
reviewed
Sep 29, 2023
Comment on lines
65
to
68
| <!-- 4 span inside the <p>. However, since div isn't a sibling of spans, | ||
| it creates a new counter. | ||
| it creates a new counter, which is allowed to access the 13 from the | ||
| counter-reset on the body as per example at: | ||
| https://drafts.csswg.org/css-contain/#containment-style |
There was a problem hiding this comment.
This can seem to mean that the new counter is instantiated on the div. Maybe
<!-- The <span> aren't allowed to modify the counter instantiated on the <body>,
so a new instance of the counter is created.
Since the <div> isn't a sibling of the <span>, it will not inherit this new instance,
and instead it will read the original counter from the <body>, with value 13.
This is per example at: https://drafts.csswg.org/css-contain/#containment-style
Also noting that even if the div were a sibling of the span, I think that currently it wouldn't inherit the nested counter due to #5477, but this was a bad decision that I want to revert.
tabatkins
reviewed
Sep 29, 2023
| @@ -63,9 +63,11 @@ | |||
| <p> <span></span> <span></span> <span></span> <span></span> | |||
|
|
|||
| <!-- 4 span inside the <p>. However, since div isn't a sibling of spans, | |||
| it creates a new counter. | |||
| it creates a new counter, which is allowed to access the 13 from the | |||
There was a problem hiding this comment.
The GH UI for suggestions is broken so I can't actually drop a suggestion here, but the text should instead say:
However, since div isn't a sibling of the spans,
it sees the original counter value on body, as per example at:
tabatkins
approved these changes
Sep 29, 2023
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-4836162
branch
2 times, most recently
from
489ed01
to
6660f02
Compare
As per: https://drafts.csswg.org/css-contain/#example-6932a400 and w3c/csswg-drafts#9116 counter() and counters() can cross the style containment boundary, so the result should be 13 from the body's counter-reset. Bug: 990657 Change-Id: Iee313a5ed6f487865b1b7c754aa45b736f6b8f15 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4836162 Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org> Cr-Commit-Position: refs/heads/main@{#1204571}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-4836162
branch
from
6660f02
to
cc4407e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As per:
https://drafts.csswg.org/css-contain/#example-6932a400
and
w3c/csswg-drafts#9116
counter() and counters() can cross the style containment boundary,
so the result should be 13 from the body's counter-reset.
Bug: 990657
Change-Id: Iee313a5ed6f487865b1b7c754aa45b736f6b8f15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4836162
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204571}