1577052 - remove manual PartialEq impl for LengthPercentage

Status: RESOLVED INVALID

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

Description

[Cameron McCormack (:heycam)]

I played around with some <position>-related animations involving calc and didn't notice anything wrong after this change.

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bff116db39349a6e47a20c39819ee2d9e4069da

Comment 2

[Cameron McCormack (:heycam)]

OK, the try run failed with a bunch of cases like:

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | radius-valued property border-bottom-left-radius: interpolation of radius with calc() units - got "25px 50px", expected "10px 20px"

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Cam said he didn't have time to look at those... I think I can try while I rest from debugging resize events :)

Comment 4

[Emilio Cobos Álvarez [:emilio]]

So I took a look, and the test fails because the value differs, and thus we stop the transition.

That happens because:

calc(25px).to_computed_value(..) -> LengthPercentage { length: 25px, percentage: 0, has_percentage: false, clamping_mode: NonNegative, was_calc: true }

But from_computed_value(calc(25px)) returns Length::Absolute(25px).

Then 25px.to_computed_value(..) -> LengthPercentage { length: 25px, percentage: 0, has_percentage: false, clamping_mode: All, was_calc: false }

Now, was_calc shouldn't even exist in the first place. There's no reason why we have behavior differences based on it. We have two uses of it and both are extremely fishy / wrong.

The clamping_mode is kind of an implementation detail, in the sense that we set it to All when we know we don't have to clamp already...

Comment 5

[Cameron McCormack (:heycam)]

We probably don't want to do this given the current representation we have for computed::LengthPercentage.