1385140 - stylo: superfluous calc() expressions appear when getting underlying background-size

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Brian Birtles (:birtles)]

Running dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html on a Stylo build from yesterday's m-c, I get the following failure:

  assert_equals: value for 'backgroundSize' on ComputedKeyframe #0 after updating current style expected "30px auto, 40% auto, auto auto" but got "calc(30px) auto, calc(0px + 40%) auto, auto auto"

It would seem like Stylo fails to simplify these calc() expressions but bug 1350739 suggests Stylo does *more* simplification of calc() than Gecko, not less, so perhaps there's something wrong with the way we're serializing.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is intended and follows the spec as far as I can tell. In specified values you're able to simplify as much as possible preserving the calc-ness.

Comment 2

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> This is intended and follows the spec as far as I can tell. In specified
> values you're able to simplify as much as possible preserving the calc-ness.

Something is whacky here. In this test we have:

  expected[0].backgroundSize = div.style.backgroundSize =
    "30px auto, 40% auto, auto auto";

So it seems like we're adding calc() in.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Oh, ok, yeah, that definitely looks fishy then, I agree.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Pretty sure this is because in LengthOrPercentageOrAuto::add_weighted we don't do the `is_definitely_zero` checks that we do in LengthOrPercentage::add_weighted.

Comment 5

[Brian Birtles (:birtles)]

Ah, that would make sense, assuming we're doing an add with zero somewhere.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Should be easy to fix, actually... let me just take this.

Comment 7

[Brian Birtles (:birtles)]

Thanks Emilio!

(Note that unfortunately this bug alone won't be enough to get test_keyframeeffect-getkeyframes.html to pass. We also need to fix bug 1385139, bug 1370779, bug 1377541 or some part thereof, and do something about text-shadow serialization for that. It should get one of the subtests to pass, however.)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1385140: Don't unconditionally generate Calc values when converting from Gecko.

Review commit: https://reviewboard.mozilla.org/r/162348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162348/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Took more to build and verify it fixed it than to write it... oh well.

Comment 10

[Brian Birtles (:birtles)]

I'm still trying to work out why add_weighted gets called from GetKeyframes().

As I understand it for this case we have an empty 'from' keyframe so we'll leave mServoDeclarationBlock as null and fill it in in GetKeyframes from the base styles. We fetch those base styles using Servo_ComputedValues_ExtractAnimationValue and currently I don't see anywhere there that calls add_weighted. I guess I need to step through this to see.

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8891159 [details]
Bug 1385140: Don't unconditionally generate Calc values when converting from Gecko.

https://reviewboard.mozilla.org/r/162348/#review167652

This doesn't fix the bug for me; test_keyframeeffect-getkeyframes.html still fails (and as per comment 10 I don't understand how this could fix it). The patch also needs a description of what problem it is fixing and how.

Comment 12

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Took more to build and verify it fixed it than to write it... oh well.

Took me even longer still to review it ;)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #11)
> Comment on attachment 8891159 [details]
> Bug 1385140: Don't create a calc expression for animation if one of the
> values is superfluous.
> 
> https://reviewboard.mozilla.org/r/162348/#review167652
> 
> This doesn't fix the bug for me; test_keyframeeffect-getkeyframes.html still
> fails (and as per comment 10 I don't understand how this could fix it). The
> patch also needs a description of what problem it is fixing and how.

Hmm... You're completely right, I accidentally tested it with a build that had stylo disabled, such a shame :)

Now I self-assigned it to me, so I'll investigate. Sorry for the hassle.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891159 [details]
Bug 1385140: Don't unconditionally generate Calc values when converting from Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162348/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I just moved it to https://github.com/servo/servo/pull/17906, feel free to review either here or there.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> I just moved it to https://github.com/servo/servo/pull/17906, feel free to
> review either here or there.

Thanks Emilio! Looks good. I guess the patch in this bug still makes sense though? (Although I'd have to think through the auto + 0 behavior.)

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8891159 [details]
Bug 1385140: Don't unconditionally generate Calc values when converting from Gecko.

https://reviewboard.mozilla.org/r/162348/#review168050

Hiro has already reviewed this on the PR but just for completeness :)

Comment 18

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/d24b801a5c8e