Closed Bug 1350739 Opened 7 years ago Closed 6 years ago

stylo: Stylo simplifies calc() expressions, Gecko doesn't.

Categories

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

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

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox57 --- wontfix

People

(Reporter: emilio, Unassigned)

References

(Blocks 2 open bugs)

Details

See https://github.com/servo/servo/pull/16144#issuecomment-289310428, Servo seems to be correct per the CSS values spec as of right now.

> To serialize a calc() value:
>   Simplify the expression by:
>     Replacing nested calc() values with parentheses containing their contents
>     Resolving all multiplications and divisions
>     Combining identical units
>
>   The result must be a summation of unique units. (Terms with a value of zero
>   must be preserved in this summation.)
>
>   If this simplification process results in only a single value (one <number>,
>   one <dimension>, or one <percentage>), and the value being serialized is a
>   computed value or later, serialize it just as that one value, without the
>   calc() wrapper. If this value is outside the allowed range for the context,
>   it must be clamped to the nearest allowed value.
>
>   Otherwise, serialize as a calc() containing the summation, with the units
>   ordered ASCII case-insensitive alphabetically, the number (if present)
>   coming before all units, and the percentage (if present) coming after all
>   units.

Simple test:

<!doctype html>
<div style="line-height: calc(1px + 2px);"></div>
<script>
  alert(document.querySelector('div').style.lineHeight);
</script>

Blink and Stylo alert "calc(3px)", Gecko alerts "calc(1px + 2px)".

With integers is funnier:

<!doctype html>
<div style="line-height: calc(1 + 2);"></div>
<script>
  alert(document.querySelector('div').style.lineHeight);
</script>

Blink: 3
Stylo (before that PR): 3
Stylo (after that PR): calc(3)
Gecko: calc(1 + 2)

I believe we want to go with that PR and file a bug against chromium.
I have some patches for bug 1350069 that makes Gecko behave more like
Blink for <integer> and <number> calc() expressions.  So "calc(1 + 2)"
would compute to "3".

I believe the spec allows for this behavior, with some restrictions:
https://drafts.csswg.org/css-values/#calc-computed-value
(In reply to Mats Palmgren (:mats) from comment #1)
> I believe the spec allows for this behavior, with some restrictions:
> https://drafts.csswg.org/css-values/#calc-computed-value

I don't think it allows for that in the test case I'm mentioning, given element.style.lineHeight is a specified value, not a computed value, right?

> If this simplification process results in only a single value (one <number>, one <dimension>, or one <percentage>), and the value being serialized is a computed value or later, serialize it just as that one value, without the calc() wrapper. If this value is outside the allowed range for the context, it must be clamped to the nearest allowed value.

In particular, note the _and the value being serialized is a computed value or later_. Otherwise, we should go to the next step, which would be:

> Otherwise, serialize as a calc() containing the summation, with the units ordered ASCII case-insensitive alphabetically, the number (if present) coming before all units, and the percentage (if present) coming after all units.

(Which would produce "calc(3)" if I understood it correctly).
Right, in your case it's bullet 3 that applies.  For a computed value, I think
it's bullet 2, i.e. "without the calc() wrapper".  (I was only talking about
computed values, sorry for the confusion.)
Hmm, I just tested your case with my gecko patches:
<div style="initial-letter: calc(1 + 1)"><x></x></div>
<script>
  alert(document.querySelector('div').style.initialLetter);
</script>

results in "2", fwiw.
Probably doesn't impact that many tests or pages, but may require spec or alignment work, so p2.
Priority: -- → P2
See Also: → 1296209, 1350069
See Also: → 1385140
Priority: P2 → --
Priority: -- → P3
See Also: → 1396692
Emilio, are we planning to ship this as an intentional behavior change?
Flags: needinfo?(emilio)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> Emilio, are we planning to ship this as an intentional behavior change?

Yes. Stylo is more conformant with the spec and in general we've had no web compat issues due to this. Plus the behavior matches Blink's / Webkit's, so I think we're good.
Flags: needinfo?(emilio)
Blocks: stylo-behavior-changes
No longer blocks: 1337166
Priority: P3 → P5
> Plus the behavior matches Blink's / Webkit's, so I think we're good.

Not quite.  Blink/Webkit serialize "calc(0.8 - 0.2)" to "0.6" whereas
Stylo serialize it to "calc(0.6)", so there's a (small) web-compat risk
in shipping this before Chrome does.  Granted, Stylo does what the spec
says, but it might be a good idea to figure out (from actual Chrome
developers - not Google's representatives on the CSSWG) if they
actually intend to implement this, because I wouldn't be surprised
if they never will, which increasingly creates web-compat issues for
other UAs that do.
(In reply to Mats Palmgren (:mats) from comment #8)
> > Plus the behavior matches Blink's / Webkit's, so I think we're good.
> 
> Not quite.  Blink/Webkit serialize "calc(0.8 - 0.2)" to "0.6" whereas
> Stylo serialize it to "calc(0.6)", so there's a (small) web-compat risk
> in shipping this before Chrome does.  Granted, Stylo does what the spec
> says, but it might be a good idea to figure out (from actual Chrome
> developers - not Google's representatives on the CSSWG) if they
> actually intend to implement this, because I wouldn't be surprised
> if they never will, which increasingly creates web-compat issues for
> other UAs that do.

Yeah, that's https://bugs.chromium.org/p/chromium/issues/detail?id=705300, which I filed a while ago and it's only for some kind of values (<integer> IIRC).

In any case, implementing that behavior in stylo is a matter of removing code, so should be trivial-ish if we found any compat issues, and they apparently plan to implement when CSS values Level 4 is published.
See Also: → https://bugs.chromium.org/p/chromium/issues/detail?id=705300
> it's only for some kind of values (<integer> IIRC).

All <number>s I think:
data:text/html,<body style="opacity:calc(0.8 - 0.2)" onload="alert(document.body.style.opacity)">
but yeah, they don't do it for <length>.

Thanks for reporting the Blink bug.  But as I suspected, they seem to prefer
their current behavior over what the spec says.  I wouldn't be at all
surprised if they pressure the CSSWG to change the spec to be compatible
with Chrome in the end.
(In reply to Mats Palmgren (:mats) from comment #10)
> > it's only for some kind of values (<integer> IIRC).
> 
> All <number>s I think:
> data:text/html,<body style="opacity:calc(0.8 - 0.2)"
> onload="alert(document.body.style.opacity)">
> but yeah, they don't do it for <length>.

You're right.

> Thanks for reporting the Blink bug.  But as I suspected, they seem to prefer
> their current behavior over what the spec says.  I wouldn't be at all
> surprised if they pressure the CSSWG to change the spec to be compatible
> with Chrome in the end.

I think it's hard to justify doing it for <length> and not for <number>, so I have hope on this one. But of course ICBW :)
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
This is working per spec, so I'm resolving WFM for now, we can track any issues this behavior causes in the bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.