Closed Bug 1430817 Opened 6 years ago Closed 6 years ago

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

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 6 obsolete files)

41 bytes, text/x-github-pull-request
MatsPalmgren_bugz
: feedback+
Details | Review
3.28 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
13.06 KB, patch
MatsPalmgren_bugz
: feedback+
Details | Diff | Splinter Review
7.65 KB, patch
MatsPalmgren_bugz
: feedback+
Details | Diff | Splinter Review
9.40 KB, patch
MatsPalmgren_bugz
: feedback+
Details | Diff | Splinter Review
21.14 KB, patch
MatsPalmgren_bugz
: feedback+
Details | Diff | Splinter Review
496.44 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
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;
Priority: -- → P3
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
I'll take this one, working on it right now.
Assignee: nobody → 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)
https://github.com/servo/servo/pull/19850 has some preliminar cleanup.
Attached file 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.
Attachment #8945052 - Flags: feedback?(mats)
(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 :)
Blocks: 1432154
(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.
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 :)
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
Attachment #8945052 - Flags: feedback?(mats) → feedback+
Attachment #8945957 - Attachment is obsolete: true
Attachment #8945957 - Flags: review?(mats)
Attachment #8945958 - Attachment is obsolete: true
Attachment #8945958 - Flags: review?(mats)
Attachment #8945959 - Attachment is obsolete: true
Attachment #8945959 - Flags: review?(mats)
I have zero idea about what has mozreview done here,  wow.
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.
Attachment #8945960 - Attachment is obsolete: true
Attachment #8945961 - Attachment is obsolete: true
Attachment #8945962 - Attachment is obsolete: true
Attachment #8945962 - Flags: review?(mats)
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".
Attachment #8945966 - Flags: review?(mats)
(Let me know if you're not confident reviewing Rust code. I'll ask :nox to take a look too on servo/servo anyway)
Attachment #8945967 - Flags: review?(mats)
(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.
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.
Attached patch Update testsSplinter Review
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
Attachment #8945971 - Flags: review?(mats)
Attachment #8945970 - Flags: review?(mats)
Blocks: 1432148
Blocks: 1363875
Attachment #8945966 - Flags: review?(mats) → review+
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.
Attachment #8945967 - Flags: review?(mats) → feedback+
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.
Attachment #8945968 - Flags: review?(mats) → feedback+
Comment on attachment 8945969 [details] [diff] [review]
Fix the <overflow-position> position, and remove fallback.

(feedback+ with same caveat as above)
Attachment #8945969 - Flags: review?(mats) → feedback+
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.
Attachment #8945970 - Flags: review?(mats) → feedback+
Attachment #8945971 - Flags: review?(mats) → review+
(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.)
Pushed by ecoal95@gmail.com:
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
Pushed by ecoal95@gmail.com:
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
Pushed by ecoal95@gmail.com:
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
Keywords: dev-doc-needed
Depends on: 1434314
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.
(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.
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!
Flags: needinfo?(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.
Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: