1430817 - [css-align] Update place-content, align-content, justify-content to spec

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

align-content and justify-content now have different set of values:
https://drafts.csswg.org/css-align-3/#align-justify-content
(left/right is only valid in justify-content, and <baseline-position>
is only valid in align-content)

We currently allow all values in both longhands (according to an
older version of the spec).

Changing this also affects place-content obviously - when a single
value is specified it should only be accepted when it's a valid
value for both longhands.

Examples of invalid values:
align-content: left;
place-content: left;
justify-content: baseline;
place-content: baseline;

Comment 1

[Javier Fernandez]

I have removed the "left/right" values in Blink and WebKit:

  - https://bugs.chromium.org/p/chromium/issues/detail?id=802095
  - https://bugs.webkit.org/show_bug.cgi?id=181792

Now working on removing the <baseline-position> values here:

  - https://bugs.chromium.org/p/chromium/issues/detail?id=802248

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'll take this one, working on it right now.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(I'm only doing this on stylo btw, I don't think it's worth to do twice the work since we're going to ship stylo on android and chrome on 60)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/19850 has some preliminar cleanup.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Actual patch

This is the actual patch. Need to run it through try and such (doing that now), but if you could take a look it'd be great!

Let me know if you prefer mozreview for this stuff too, I probably need to submit the test expectations via there anyway.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Let me know if you prefer mozreview for this stuff too, I probably need to
> submit the test expectations via there anyway.

And the new tests of course :)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Created attachment 8945052 [details] [review]
> Actual patch
> 
> This is the actual patch. Need to run it through try and such (doing that
> now), but if you could take a look it'd be great!
> 
> Let me know if you prefer mozreview for this stuff too, I probably need to
> submit the test expectations via there anyway.

Just noticed that I also need to remove fallback alignment and such, on it.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Still tons of tests to update, but what is on the PR plus a patch for serializing <overflow-position> before the rest of the stuff in nsCSSValue should get us most of the way there.

Mats, for now I'm removing the subtests testing align-*: left | right; and justify-*: baseline *;, and commenting out the ones that test fallback values given they're likely to be added back to the spec.

Any objection to that? Just checking beforehand just in case :)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9194e5f7b753133732fb197249114075b58a9618&selectedJob=158284123 (with all the orange)

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8945052 [details] [review]
Actual patch

> Mats, for now I'm removing the subtests testing align-*: left | right; and
> justify-*: baseline *;, and commenting out the ones that test fallback
> values given they're likely to be added back to the spec.

Sounds good to me.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1430817: Serialize <overflow-position> before other align bits.

Otherwise the serialisation wouldn't roundtrip with the new syntax, which fixes
the position of <overflow-position>.

Also make Servo and Gecko agree on whether to serialize "unsafe".

Review commit: https://reviewboard.mozilla.org/r/216022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216022/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Make content distribution parsing know the axis it's parsed for.

Review commit: https://reviewboard.mozilla.org/r/216024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216024/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Restrict <baseline-position> and <content-position> depending on the axis in content distribution properties.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1430817, and updates us
to the current version of the css-align spec.

Review commit: https://reviewboard.mozilla.org/r/216026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216026/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Fix the position of the <overflow-position> in content distribution shorthands, and remove fallback.

Review commit: https://reviewboard.mozilla.org/r/216028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216028/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Update align-self / justify-self to the spec too.

Review commit: https://reviewboard.mozilla.org/r/216030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216030/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1430817: Update tests.

Review commit: https://reviewboard.mozilla.org/r/216032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216032/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945960 [details]
style: Fix the position of the <overflow-position> in content distribution shorthands, and remove fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216028/diff/1-2/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945961 [details]
style: Update align-self / justify-self to the spec too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216030/diff/1-2/

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945962 [details]
Bug 1430817: Update tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216032/diff/1-2/

Comment 20

[Emilio Cobos Álvarez (:emilio)]

I have zero idea about what has mozreview done here,  wow.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8945960 [details]
style: Fix the position of the <overflow-position> in content distribution shorthands, and remove fallback.

I'll just attach patches, sigh. The Github thing is up-to-date too.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Serialize <overflow-position> before other align bits.

Otherwise the serialisation wouldn't roundtrip with the new syntax, which fixes
the position of <overflow-position>.
    
Also make Servo and Gecko agree on whether to serialize "unsafe".

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Make content distribution parsing know the axis it's parsed for.

(Let me know if you're not confident reviewing Rust code. I'll ask :nox to take a look too on servo/servo anyway)

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Restrict <baseline-position> and left | right depending on the axis direction.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> Created attachment 8945968 [details] [diff] [review]
> Restrict <baseline-position> and left | right depending on the axis
> direction.

Just noticed that this commit has a subtle bug that is fixed later on (intersects() vs. contains() on the flags). Will rejigger the commits before landing so that it doesn't land on a partial state.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Fix the <overflow-position> position, and remove fallback.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Attachment: Update align-self / justify-self / place-self to the spec too.

After this the only spec non-conformance issue left is the justify-items: auto value, which is still default.

The reason for that is because it has special handling in both the old and the new style system, so it's a more tricky change that I prefer to do separately.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Attachment: Update tests

May need a few

disabled:
  if not stylo

on the WPT tests, but rest is green everywhere.

The remaining failures in WPT are just due to the serialization issue[1]. I'll submit a few fixes for that directly to the w3c repo as a followup, since there it can be reviewed by the Igalia people more easily, presumably :).

Sorry for the large patch, this is actually what took the most time, d'oh.

[1]: https://github.com/w3c/csswg-drafts/issues/2229

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 8945967 [details] [diff] [review]
Make content distribution parsing know the axis it's parsed for.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #23)
> (Let me know if you're not confident reviewing Rust code. I'll ask :nox to
> take a look too on servo/servo anyway)

Yeah, this patch far exceeds my level of Rust knowledge.
This looks like a bug though:

>+        let align_content =
>+            ContentDistribution::parse(
>+                input,
>+                FallbackAllowed::No,
>+                AxisDirection::Inline,

---------------------------------^^^^^^ should be Block?

>+        let justify_content = input.try(|input| {
>+            ContentDistribution::parse(
>+                input,
>+                FallbackAllowed::No,
>+                AxisDirection::Block,
---------------------------------^^^^^ should be Inline?

(It's later fixed in part 4 though, so don't bother updating
this patch unless you feel like it.)

The rest looks fine if I'm guessing correctly what the code does, fwiw.

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 8945968 [details] [diff] [review]
Restrict <baseline-position> and left | right depending on the axis direction.

Looks fine with my limited Rust knowledge, fwiw.
But please get a proper Rust code review here too.

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 8945969 [details] [diff] [review]
Fix the <overflow-position> position, and remove fallback.

(feedback+ with same caveat as above)

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 8945970 [details] [diff] [review]
Update align-self / justify-self / place-self to the spec too.

LGTM FWIW.

>+        if self.primary.contains(AlignFlags::BASELINE) ||
>+            self.primary.contains(AlignFlags::LAST_BASELINE)

Is the Rust compiler allowed (and smart enough) to optimize
this to a single bitwise-or op?  If not, then it might be
worth adding a has_any function to do that.

Comment 33

[Mats Palmgren (inactive)]

(For the record: I also noted that the parsing functions in the old style system
code weren't updated to the new syntax, but I assume we don't do that anymore.)

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/715eee480c0f
Serialize <overflow-position> before other align bits. r=mats
https://hg.mozilla.org/integration/autoland/rev/a257c8b3582a
Update tests. r=mats

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e635716b691e
Backout changeset 715eee480c0f for landing with the wrong author information. r=backout
https://hg.mozilla.org/integration/autoland/rev/6ddf262f75e9
Serialize <overflow-position> before other align bits. r=mats

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b908d1296ebf
followup: Skip some more tests in non-stylo, since it's not updated to the spec yet. r=me

Comment 37

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a257c8b3582a
https://hg.mozilla.org/mozilla-central/rev/6ddf262f75e9
https://hg.mozilla.org/mozilla-central/rev/b908d1296ebf

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, the way you upstreamed these broke my script to check if I need to upstream the tests, so it's going to warn me every day that this commit needs to be upstreamed until I upstream something else.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚️UTC (busy until February 5) from comment #40)
> FWIW, the way you upstreamed these broke my script to check if I need to
> upstream the tests, so it's going to warn me every day that this commit
> needs to be upstreamed until I upstream something else.

Ah, sorry. I did ask in #layout and dholbert said it was fine to update both.

Anyway, I'll resurrect some of the tests in bug 1434314, so will make sure to not touch the wpt ones.

Comment 42

[Chris Mills [:cmills]]

I've updated the docs to reflect the new syntax changes:
https://developer.mozilla.org/en-US/docs/Web/CSS/align-content
https://developer.mozilla.org/en-US/docs/Web/CSS/justify-content
https://developer.mozilla.org/en-US/docs/Web/CSS/place-content

I've also updated the relevant data repos from which we generate the formal syntax and the compat data:
https://github.com/mdn/browser-compat-data/pull/1173
https://github.com/mdn/data/pull/180

And finally, I've added a note to the Fx60 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/60#CSS

Let me know if this looks OK. Thanks!

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Looks good! Note that this bug also removed left | right from the other block axis properties. That is, this bug also fixed bug 1432154.