We incorrectly skip 0%s in transform serialization.
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
People
(Reporter: emilio, Assigned: aw4910819, Mentored)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [lang=rust], [wptsync upstream])
Attachments
(1 file)
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Just noticing that this same issue seems to affect the translate:;
property as well as transform: translate();
/* input CSS */
one value {
translate: 1px;
}
two values {
translate: 1px 0;
}
/* parsed in CSSOM */
one value {
translate: 1px;
}
two values {
translate: 1px;
}
This is present in Firefox Nightly 73.0b2 (64-bit) on MacOS, though also in every version of Firefox I check as well so I don't believe it's specific to this build. The behaviour seems the same as when transform: translate(1px, 0)
is parsed.
Reporter | ||
Comment 2•5 years ago
|
||
This is about 0%
. Skipping 0px
is correct, including for the individual transform, see https://drafts.csswg.org/css-transforms-2/#individual-transform-serialization
Comment 3•5 years ago
|
||
It does the same behaviour for unitless 0
and 0%
as well:
/* input */
unitless { translate: 1px 0; }
percent { translate: 1px 0%; }
/* parsed in CSSOM */
unitless { translate: 1px; }
percent { translate: 1px; }
Reporter | ||
Comment 4•5 years ago
|
||
Ok, sure... Yeah, it affects all the properties where we do skipping-if-zero... But comment 0 didn't have any percent :-)
Reporter | ||
Comment 5•5 years ago
|
||
I was going to tag this as a good-first-bug, but Chromium agrees with us in translate
... So a bit trickier.
Anyhow, to fix this, we should replace the "Zero::is_zero"
here (which right now calls is_definitely_zero
, which returns true for 0%
) for something like LengthPercentage::is_zero_length
, which should do something like !has_percentage && is_definitely_zero
or such. May need to define that method in a trait that applies both to the specified and computed LengthPercentage
.
That does it for transform: translate()
. We should also change the y.is_zero()
call here for a call to the same method to affect the translate
property.
A simple test-case is evaluating this on the terminal:
document.body.style.transform = "translate(1px, 0%)"
document.body.style.transform // Should be "translate(1px, 0%)"
Then we'd need to convert that in an automated test or what not.
I'd be interested in taking this on as a first bug, but I'm unsure how to approach it from a workflow standpoint. Unless I'm mistaken, it looks like the test cases for CSS serialization like this are in servo/tests/unit/style/parsing/
, but these tests have dependencies that are checked into Servo but not checked into mozilla-central. I would assume that means development on this is expected to take place in the Servo repo, since it's not possible to submit a patch to mozilla-central that you can demonstrate will pass these tests. But then why is this a Bugzilla bug and not a Github issue?
Apologies if there's some doc I missed explaining what the relationship between these two repos and their respective workflows is besides someone occasionally merging a subtree of Servo into mozilla-central.
Reporter | ||
Comment 7•5 years ago
|
||
No problem. Those tests are not run in mozilla-central (and we've just been deleting those unit tests when they get in the way, really).
We prefer integration tests for that, and there a lot of them, both in wpt (see for example https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-transforms/parsing), and in the internal mochitests, like all the tests that use property_database.js
.
Updated•3 years ago
|
Hi, I'd do this as a first bug if no one else is working on it.
I've prepared a fix and I'm wondering who I should set as the reviewer.
Thanks!
Reporter | ||
Comment 9•2 years ago
|
||
Sure, please go ahead and submit it with r=emilio, I can take a look at the patch :)
Assignee | ||
Comment 10•2 years ago
|
||
Adds trait ZeroNoPercent to check for values that are 0 (such as 0px) but not 0%
: Enter commit message. Lines beginning with 'HG:' are removed.
Updated•2 years ago
|
Comment 11•2 years ago
|
||
Comment 12•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Description
•