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-lists-3] counter-increment and counter-reset overflow/underflow #9029

Open
vitorroriz opened this issue Jul 4, 2023 · 3 comments
Open

Comments

@vitorroriz
Copy link

vitorroriz commented Jul 4, 2023

Specification says:

UAs may have implementation-specific limits on the maximum or minimum value of a counter. If a counter reset, set, or increment would push the value outside of that range, the value must be clamped to that range.

However, gecko and blink behavior is:

counter-reset(<overflow/underflow-int-32-number>) : clamp to range
counter-increment() : ignore operation, keep current value

WebKit behavior is:

counter-reset(<overflow/underflow-int-32-number>) : clamp to range
counter-increment() : overflow/underflow

I'd like to discuss whether the expected behavior should be "clamp to range", or we should ignore the operation for the counter-increment() case and update the spec, since 2 vendors already do that.

@tabatkins
Copy link
Member

Hm, actually I think ignoring the increment makes more sense than clamping. If you're, say, incrementing by 1000 for some visual reason, having a counter suddenly end in 647 or something, rather than 000 like all the others did, would look pretty weird.

This does make the behavior slightly inconsistent with set/reset, which will indeed give you the unexpected value, but it's (slightly) more likely that increment will run into this case (due to a very large list) than set/reset (requiring you to purposely set a very large number). Plus there's no real way to infer intent from set/reset, so we can't change the value to something else close to the maximum range, while increment does have at least a little intent we can infer.

@astearns astearns added this to Wednesday in Cupertino F2F Agenda Jul 16, 2023
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-lists-3] counter-increment and counter-reset overflow/underflow, and agreed to the following:

  • RESOLVED: counter-increment is ignored on overflow
  • RESOLVED: counter-increment will ignore the increment if it underflows or overflows
The full IRC log of that discussion <emilio> vitorroriz: currently spec says that if counter-increment overflow should clamp
<myles> q+ to clarify "overflow
<emilio> ... FF and chrome ignore the increment instead
<emilio> ... WebKit behavior is arguably a bug, so I was looking at how to fix this
<emilio> myles: just clarifying that overflow is not about layout overflow, just integer overflow, right?
<Rossen_> q?
<Rossen_> ack myles
<Zakim> myles, you wanted to clarify "overflow
<emilio> [multiple]: Yes
<myles> s/, right?//
<emilio> TabAtkins: I think I like the ignore behavior more than clamping for incrementing
<emilio> ... if you do counter-increment: 1billion, and you'd eventually get to INT32_MAX
<emilio> ... seems better to stick to <last-valid-increment>
<emilio> ... for set/reset I think clamping makes more sense
<fantasai> s/INT32_MAX/INT32_MAX, which looks weird/
<emilio> ... I don't have a strong opinion on this
<emilio> fantasai: I support all of Tab's arguments, and that's what we should do
<Rossen_> ack fantasai
<emilio> ... doesn't seem like an implementation problem
<emilio> TabAtkins: proposed resolution is "do the Gecko and Blink behavior"
<emilio> ... so change the spec so that increment ignores rather than clamps
<emilio> RESOLVED: counter-increment is ignored on overflow
<emilio> RESOLVED: counter-increment will ignore the increment if it underflows or overflows

@vitorroriz
Copy link
Author

I think this should get "needs edit" label. @tabatkins

webkit-commit-queue pushed a commit to vitorroriz/WebKit that referenced this issue Aug 11, 2023
https://bugs.webkit.org/show_bug.cgi?id=258730
rdar://111743827

Reviewed by Chris Dumez.

We will ignore the counter-increment() operation if
this would result in overflow/underflow for compatibility
with other vendors (w3c/csswg-drafts#9029).

 * LayoutTests/imported/w3c/web-platform-tests/css/css-lists/counter-reset-increment-overflow-underflow-expected.html: Added.
 * LayoutTests/imported/w3c/web-platform-tests/css/css-lists/counter-reset-increment-overflow-underflow-ref.html: Added.
 * LayoutTests/imported/w3c/web-platform-tests/css/css-lists/counter-reset-increment-overflow-underflow.html: Added.
 * Source/WTF/wtf/CheckedArithmetic.h:
 (WTF::sumIfNoOverflowOrFirstValue):
 * Source/WebCore/rendering/CounterNode.cpp:
 (WebCore::CounterNode::computeCountInParent const):

Canonical link: https://commits.webkit.org/266817@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

4 participants