1517511 - Simplify storage of computed::LengthOrPercentage and friends.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

So I was looking at bug 1516454, which caused some test failures, and realized that the failure is that we don't properly preserve the clamping mode when going from Gecko's representation of calc to Servo's.

Then I realized that I could spot-fix that one, but that it was kind of a footgun. So I instead decided to share the representation of LengthOrPercentage between Rust and C++ [1].

That's almost green, though it was a bit more annoying to write than what I expected, mostly because of how we represent it[2], which means that we need to handle lengths and percentages twice.

So it looks to me that there should be no difference in any property between calc(50%) and 50%, for example, so we should be able to represent it as CalcLengthOrPercentage, effectively.

I gave that a try[3], and my naive patch doesn't try to store length or percentages as other than calc() values. It does uncover quite a few issues with our calc() handling in layout and graphics, even assertions.

I'll try to go through some and fix them and then see what remains / if I can land it...

All these issues are reproducible without my patch, by replacing <fixed-length> by calc(<fixed-length>) or <fixed-percentage> by calc(<fixed-percentage>).

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5782de3fdcfe5aaf32770ca735f6c751ca747535
[2]: https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/servo/components/style/values/computed/length.rs#360
[3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f896a195a48e0db31b9662b03ee96c794764632b

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is all so broken :(

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1517511 - Simplify computed::LengthOrPercentage and friends. r=heycam

This is a first step to share LengthOrPercentage representation between Rust and
Gecko.

We need to preserve whether the value came from a calc() expression, for now at
least, since we do different things depending on whether we're calc or not right
now. See https://github.com/w3c/csswg-drafts/issues/3482 and dependent bugs for
example.

That means that the gecko conversion code needs to handle calc() in a bit of an
awkward way until I change it to not be needed (patches for that incoming in the
next few weeks I hope).

I need to add a hack to exclude other things from the PartialEq implementation
because the new conversion code is less lossy than the old one, and we relied on
the lousiness in AnimationValue comparison (in order to start transitions and
such, in [1] for example).

I expect to remove that manual PartialEq implementation as soon as I'm done with
the conversion.

The less lossy conversion does fix a few serialization bugs for animation values
though, like not loosing 0% values in calc() when interpolating lengths and
percentages, see the two modified tests:

 * property-types.js
 * test_animation_properties.html

Comment 3

[Emilio Cobos Álvarez (:emilio)]

[1] in the commit message is https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/layout/style/nsTransitionManager.cpp#517

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/624782f944fc
Simplify computed::LengthOrPercentage and friends. r=heycam

Comment 5

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14734 for changes under testing/web-platform/tests

Comment 6

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b7071abf6ea
followup: fix servo build.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c8bcb1cf3e7
bug 1518098 - followup: Rustfmt so Servo's lints are happy.

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/624782f944fc

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6b7071abf6ea
https://hg.mozilla.org/mozilla-central/rev/5c8bcb1cf3e7