Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-counter-styles-3] system for cjk-earthly-branch and cjk-heavenly-stem #8596

Closed
vitorroriz opened this issue Mar 15, 2023 · 11 comments
Closed
Labels

Comments

@vitorroriz
Copy link

Hi everybody, #4570 changed both styles's systems from alphabetic to fixed, however WPT tests like http://wpt.live/css/css-counter-styles/cjk-heavenly-stem/css3-counter-styles-205.html still expect it to behave as alphabetic and blink/gecko/webkit also behave as alphabetic.

Should the WPT related tests be updated and should engines change behavior or is it something still up to debate?

@vitorroriz
Copy link
Author

It seems that WPT tests were modified by blink here to match legacy behavior:
web-platform-tests/wpt@2f9d75e

@tabatkins
Copy link
Member

Ugh, that was an extremely bad test change. Explicitly testing for spec-violating behavior, while leaving the assert unchanged so it now claims the test is showing something entirely different, and all without any report to the WG asking for a change.

I'll get this fixed.

@tabatkins
Copy link
Member

Oh and for context, #2096 changed the spec from the legacy alphabetic behavior to the current fixed behavior at the explicitly request of the CLREQ people. (https://www.w3.org/2019/12/06-clreq-minutes.html#x03 is the minutes where they discussed this.)

@nt1m nt1m added the css-counter-styles-3 Current Work label May 26, 2023
@dbaron
Copy link
Member

dbaron commented Jun 14, 2023

It seems that WPT tests were modified by blink here to match legacy behavior:

For what it's worth, I think it looks more accurate to say that that change converted tests from manual tests (which are rarely if ever run) into reftests. I don't see any sign that anything was intentionally changed from one behavior to another, but it's possible the creation of the references was done incorrectly given the existing tests (although I don't see where, given that the references can be internally verified without consulting the tests).

@vitorroriz
Copy link
Author

vitorroriz commented Jun 14, 2023

Hi @dbaron I don't oppose to this change and WebKit has adopted the same behavior. I was just referring the commit message that says:

The two counter styles are officially specified as 'fixed', supporting
only a fixed range. However, the legacy implementation has been
supporting them as alphabetic counter styles. So this patch extends
their ranges to match the existing behavior.

@tabatkins
Copy link
Member

Yeah, the manual tests clearly expected the spec behavior (as documented by the meta-assert message, which still maintains that the spec is correct and contradicts the refs). The refs were generated off of Chrome's incorrect behavior, and the commit message clearly indicates that this was an intentional change.

(That said, I talked to Xiaocheng and he doesn't remember why he did this. ^_^ The tests just need to be fixed.)

@dbaron
Copy link
Member

dbaron commented Jun 14, 2023

I'm not disagreeing about whether the tests are currently right or wrong, but I don't see how anything has changed the expectations of these tests since they were originally created in web-platform-tests/wpt@7989523 (at least for -201 and -202 and -205, which I followed the full history of). I think Xiaocheng's change was just creation of references for existing tests that was consistent with what those tests already expected. (The manual tests already had internal references within the tests for manual comparison, they just weren't set up as reftests.)

@vitorroriz
Copy link
Author

Maybe another issue would be better for this but, if the system is to be fixed, is the default fallback (decimal) the best option we have? I'm not an expert, so ignore me if I'm totally wrong, but if I had to guess I'd say cjk-decimal would make more sense as a fallback (?).

@nt1m nt1m changed the title [css-counter-styles-3] cjk-earthly-branch and cjk-heavenly-stem [css-counter-styles-3] system for cjk-earthly-branch and cjk-heavenly-stem Jun 15, 2023
@nt1m
Copy link
Member

nt1m commented Jun 15, 2023

@vitorroriz Can you file a new issue for the fallback?

@vitorroriz
Copy link
Author

@vitorroriz Can you file a new issue for the fallback?

Sure! #8975

webkit-commit-queue pushed a commit to vitorroriz/WebKit that referenced this issue Jun 16, 2023
…have 'fixed' system (258089)

https://bugs.webkit.org/show_bug.cgi?id=258089
rdar://110796633

Reviewed by Tim Nguyen.

According to spec, cjk-earthly-branch and cjk-heavenly-stem counter-styles
should have 'fixed' system instead of 'alphabetic'.
See w3c/csswg-drafts#8596

* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-earthly-branch/css3-counter-styles-202-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-earthly-branch/css3-counter-styles-202.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-heavenly-stem/css3-counter-styles-205-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-heavenly-stem/css3-counter-styles-205.html:
* Source/WebCore/css/counterStyles.css:
(@counter-style cjk-earthly-branch):
(@counter-style cjk-heavenly-stem):

Canonical link: https://commits.webkit.org/265235@main
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-counter-styles-3] system for cjk-earthly-branch and cjk-heavenly-stem.

The full IRC log of that discussion <emilio> vitorroriz: some years ago the spec changed alphabetic to fixed
<emilio> ... so just wanted to make sure it's fixed indeed
<emilio> TabAtkins: yeah tests were just wrong
<emilio> vitorroriz: I fixed them
<emilio> TabAtkins: great, nothing on the spec needs change

mnutt pushed a commit to movableink/webkit that referenced this issue Jun 28, 2023
…have 'fixed' system (258089)

https://bugs.webkit.org/show_bug.cgi?id=258089
rdar://110796633

Reviewed by Tim Nguyen.

According to spec, cjk-earthly-branch and cjk-heavenly-stem counter-styles
should have 'fixed' system instead of 'alphabetic'.
See w3c/csswg-drafts#8596

* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-earthly-branch/css3-counter-styles-202-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-earthly-branch/css3-counter-styles-202.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-heavenly-stem/css3-counter-styles-205-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/cjk-heavenly-stem/css3-counter-styles-205.html:
* Source/WebCore/css/counterStyles.css:
(@counter-style cjk-earthly-branch):
(@counter-style cjk-heavenly-stem):

Canonical link: https://commits.webkit.org/265235@main
@astearns astearns added this to Unslotted in TPAC 2023 agenda Sep 7, 2023
@nt1m nt1m removed the Agenda+ label Sep 9, 2023
@nt1m nt1m removed this from Unslotted in TPAC 2023 agenda Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants