1573830 - We incorrectly skip 0%s in transform serialization.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1566167 comment 7.

Comment 1

[tomhodgins]

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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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

[tomhodgins]

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; }

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok, sure... Yeah, it affects all the properties where we do skipping-if-zero... But comment 0 didn't have any percent :-)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 6

[Ben Levin]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[aw4910819]

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!

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Sure, please go ahead and submit it with r=emilio, I can take a look at the patch :)

Comment 10

[aw4910819]

Attachment: Bug 1573830 - 0% values are not skipped when parsing CSS transform. r=emilio

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61281c254069
0% values are not skipped when parsing CSS transform. r=emilio

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/61281c254069

Comment 13

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

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

Comment 14

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

Upstream PR merged by moz-wptsync-bot