1514309 - "unsafe" css-align keyword should be preserved in specified style

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

We need to revert bug 1230398, effectively.

Back when we fixed that bug, the "unsafe" (aka "true") alignment was the default behavior, and was equivalent to the value not being specified.

Now it's not anymore (per spec at least), so we need to preserve the keyword when stringifying the property value for specified/computed style.

Note: It looks like this is responsible for most (maybe all?) of the *-content / *-self failures on bug 1514237, e.g. this one:
https://wpt.fyi/results/css/css-align/content-distribution/place-content-shorthand-001.html?products=chrome%5Bstable%5D%2Cfirefox%5Bexperimental%5D%2Csafari%5Bstable%5D&sha=48d5d5033d

Firefox has 2 failures in that example, due to us removing this "unsafe" keyword during a roundtrip. The failures are reported as:
> !EQ("unsafe end", "end")
> !EQ("unsafe flex-start", "flex-start")

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1514309 part 1: Include the 'unsafe' keyword in serializations of css-align properties. r?emilio

Comment 2

[Daniel Holbert [:dholbert]]

Maybe this is all we need, aside from test-expectation tweaks?

I verified locally that this test (from comment 0) starts passing, with this tweak, at least:
  http://w3c-test.org/css/css-align/content-distribution/place-content-shorthand-001.html

I'll do a test run and adjust test expectations accordingly.

(And we can make a test strictness tweak: it looks like we can remove one of the allowed shorthand syntaxes from the arrays in https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-align/default-alignment/shorthand-serialization-001.html , effectively reverting the changes in https://hg.mozilla.org/mozilla-central/rev/bbd2f0c18f64 )

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1514309 part 2: Remove C++ serialization code for CSS {align,justify}-{content,items,self} properties. r?emilio

Depends on D14599

Comment 4

[Daniel Holbert [:dholbert]]

Try run with both parts:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=019b7ef359e72328092f922db9a36e4b2714d7b6

Try run with just part 1 (just to demonstrate that we pass tests at that incremental/surgical step too):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc5d6ffba43d39d5b55e5ec2aec340e31f41784c

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Try run with both parts:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=019b7ef359e72328092f922db9a36e4b2714d7b6

(This ^^ was actually from an older version of Part 2, before I did the nsCSSProps.cpp keyword-table removals. But I did validate locally that we build just fine with the keyword tables removed, so I think those are OK to get rid of.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fe457a64309
part 1: Include the 'unsafe' keyword in serializations of css-align properties. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5e7b8cd0e526
part 2: Remove C++ serialization code for CSS {align,justify}-{content,items,self} properties. r=emilio

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4fe457a64309
https://hg.mozilla.org/mozilla-central/rev/5e7b8cd0e526

Comment 8

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

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

Comment 9

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

Upstream PR merged