Open Bug 1408257 Opened 7 years ago Updated 2 years ago

serialization of counter-reset/increment specified values should not include default value

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: heycam, Unassigned)

Details

<body style="counter-increment: abc" onload="alert(document.body.style.counterIncrement)"> This alerts "abc 1" in Firefox, Chrome and Safari, but I don't see anything in specs that say that the implied "1" value should be included in the specified value.
Edge doesn't include it.
status-firefox57: --- → wontfix
Priority: -- → P3
Similarly, `<body style="counter-reset: abc" onload="alert(document.body.style.counterReset)">` displays "abc 0". Cameron, should this also be fixed? (At first glance I see quite a bit of common code, including how the value of both properties is stored, so it may make sense to combine both fixes.)
Assignee: nobody → gsquelart
Flags: needinfo?(cam)
How important is it to fix this? We'd need to special-case all the different properties that use CounterPair, since the default value is different for each property. I'm not sure it's worth it given it also makes us incompatible with other engines.
(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #3) > How important is it to fix this? I'm a Layout newbie, and that was a "good first bug" suggestion, so I'd say importance-to-fix is not as significant as how much working on it could teach me. ;-) > We'd need to special-case all the different properties that use CounterPair, > since the default value is different for each property. I'm not sure it's > worth it given it also makes us incompatible with other engines. But if you feel this fix is in fact *undesirable*, we should instead just mark it as WONTFIX.
(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #3) > We'd need to special-case all the different properties that use CounterPair, Are there properties other than counter-increment and counter-reset that we support that would be affected? We are already using separate CounterIncrement and CounterReset types so it might not be a big deal. > since the default value is different for each property. I'm not sure it's > worth it given it also makes us incompatible with other engines. Right, incompatible with Chrome / WebKit but compatible with Edge. As Gerald says, I suggested this one just as a way to get some familiarity with code in this area. My guess is that it's not important for compatibility either way. I don't mind if we decide that the current serialization is what we want. Emilio, as the CSSOM editor now, do you think we should keep the current behavior? Step 2 in https://drafts.csswg.org/cssom/#serializing-css-values is the guidance on what to do in general when serializing values, so if we don't want to change, we should consider filing an issue on css-lists-3 to specify the serialization as always including the number (and CC @gregwhitworth to see what he thinks).
Flags: needinfo?(cam) → needinfo?(emilio)
(In reply to Cameron McCormack (:heycam) from comment #5) > (In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) > from comment #3) > > We'd need to special-case all the different properties that use CounterPair, > > Are there properties other than counter-increment and counter-reset that we > support that would be affected? We are already using separate > CounterIncrement and CounterReset types so it might not be a big deal. Hmm, yeah, I guess it's just a matter of making to_css for CounterPair a free function taking the default value. Not so terrible as I'd initially thought actually :P > > since the default value is different for each property. I'm not sure it's > > worth it given it also makes us incompatible with other engines. > > Right, incompatible with Chrome / WebKit but compatible with Edge. As > Gerald says, I suggested this one just as a way to get some familiarity with > code in this area. My guess is that it's not important for compatibility > either way. I don't mind if we decide that the current serialization is > what we want. Emilio, as the CSSOM editor now, do you think we should keep > the current behavior? Oh, didn't know Edge was doing it right. I agree it's not a big deal compat-wise, probably, and that the right thing ideally is to skip the default value.
Flags: needinfo?(emilio)
Opened csswg issue: https://github.com/w3c/csswg-drafts/issues/2635 And I've expanded this bug to also cover counter-reset, which shows the same behavior but with a different default value.
Summary: serialization of counter-increment specified values should not include default increment value → serialization of counter-reset/increment specified values should not include default value
I've just realized something: In the CSSOM general principles of serialization: https://drafts.csswg.org/cssom/#serializing-css-values > If component values can be omitted or replaced with a shorter representation without changing > the meaning of the value, omit/replace them. I interpret this as: Whether or not the default value was given by omission or explicitly, we should remove it anyway! I originally thought it was only when omitted, as per the example in comment 0. In which case, this: > <body style="counter-increment: abc 1" onload="alert(document.body.style.counterIncrement)"> (where we specify "abc 1") should *also* only alert "abc". It makes the implementation a bit simpler, as we don't need to know how the default value came to be. Note that Edge does make the distinction, and prints the default value when it was given explicitly. Emilio, could you please confirm that that's the intent of this principle? (That the default value should never be serialized, even if it was given explicitly.) What about something like 'calc(2 - 1)', or more complex, that results in the default value?
Flags: needinfo?(emilio)
For the first part of the question, yes, "abc 1" should be serialized as just "abc". And for the second part regarding calc, I think we general reserve the calc function when serializing, that is because spec-wise, calc function can wrap an invalid value which is clampped (rather than dropped) at computation time. For example, "padding: -1px" is invalid and drop at parsing, but "padding: calc(-1px)" is valid, and gets clammped to "padding: 0px" when we compute it. So for consistency, we should probably serialize 'calc(2 - 1)' to 'calc(1)' rather than dropping it for serialization.
Yes, that's exactly right, but I don't see how does it make the implementation simpler. The annoying bit is that the default values change depending on the property,so we need to split the type into two.
Flags: needinfo?(emilio)

(I'm not working in this part of the world anymore, back to the triage pile...)

Assignee: gsquelart → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.