1257688 - Make -webkit-box alignment properties aliases of legacy -moz-box properties (with custom handling in modern flexbox), not aliases of modern alignment properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Currently (with layout.css.prefixes.webkit enabled), we support a suite of properties associated with -webkit-box as aliases to modern flexbox-related properties.

However, mats brought up some concerns in bug 1231682 -- in particular:
 - The modern properties that we map these to (e.g. "justify-content") have effects beyond flexbox.

 - This means that <div style="display:grid; -webkit-box-pack:end"> would only get an alignment effect in Firefox, but not in other browsers. (which could potentially break the rendering for us, if the -webkit-box-pack styling was accidental cruft & particularly if the author didn't test in Firefox)

 - More worrisome (to me at least), once we start honoring these modern alignment properties (like justify-content) on display:block elements, then our current aliasing strategy would mean that previously-inactive-everywhere copypasted "-webkit-box-pack" cruft that authors put on blocks would all of a sudden start influencing layout in Firefox.

So: to address these concerns, we probably need to implement separate properties for -webkit-box alignment & ordering, and make our flexbox layout code use one or the other set of properties depending on our "display" value.

(I don't think this needs to block us from shipping webkit prefix-support, because for now, these properties are all de-facto specific to flexbox & grid, and I'm skeptical that any early-adopters experimenting with grid will mix in webkit-box cruft with their grid styling. We should address this before grid usage becomes widespread, though, and we definitely need to address it before we implement css-align support for blocks & tables.)

Comment 1

[Daniel Holbert [:dholbert]]

(I'm going to layer this bug on top of my patches for bug 1236400; adding dependency.)

Comment 2

[Daniel Holbert [:dholbert]]

Refinement on this plan: instead of bloating our style structs with new properties purely for legacy compatibility's sake, we should instead:
 - Change to map "-webkit-box-pack" to "-moz-box-pack", "-webkit-box-flex" to "-moz-box-flex", etc.
 - Recall that "display:-webkit-box" will make us use nsFlexContainerFrame instead of XUL.
 - Adjust nsFlexContainerFrame to use these -moz-box-* alignment/flexibility/order properties instead of the modern ones, if it sees that its display value is -webkit-box.

Comment 3

[Daniel Holbert [:dholbert]]

(So in other words: these properties would still be aliases, but they'd be aliases for our internal -moz-box related properties which aren't going to cause weird conflicts/interactions like the ones that mats was worried about with the modern alignment properties.)

Comment 4

[Daniel Holbert [:dholbert]]

[updating summary per comment 2]

Comment 5

[Daniel Holbert [:dholbert]]

Patches coming shortly.

Comment 6

[Daniel Holbert [:dholbert]]

(Also, I expect this to fix bug 1256664 & bug 1258733 -- as part of this bug, nsFlexContainerFrame will start looking at our resolved -[moz|webkit]-box-flex instead of flex-shrink to determine shrinkability -- so we won't have those bugs' problems from default "flex-shrink:1" anymore.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1257688 part 1: Change "-webkit-box-pack" & "-webkit-box-align" to alias their -moz equivalents, & change nsFlexContainerFrame to respect them in a -webkit-box. r=mats

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

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1257688 part 2: Enable "-webkit-box-pack: justify" sections of -webkit-box reftests, & fix reference cases to use 'space-between' (the modern equivalent). (no review)

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

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1257688 part 0: Add an "IsLegacyBox" accessor to nsFlexContainerFrame, to enable special handling of display:-webkit-box & display:-webkit-inline-box. r=mats

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

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1257688 part 3: Change "-webkit-box-ordinal-group" to alias its -moz equivalent, & change nsFlexContainerFrame to respect it in a -webkit-box. r=mats

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

Comment 11

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4ed986f9e4e

Patch/tests still to come for one more property here, "-webkit-box-flex".

And I'll spin off a followup bug for "-webkit-box-orient", since its fix will largely involve backing out all of the changes on bug 1208344, and that's a bit of a tangent/rathole for a broad-ish bug like this one.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8737402 [details]
MozReview Request: Bug 1257688 part 1: Change "-webkit-box-pack" & "-webkit-box-align" to alias their -moz equivalents, & change nsFlexContainerFrame to respect them in a -webkit-box. r=mats

https://reviewboard.mozilla.org/r/43953/#review40553

::: layout/generic/nsFlexContainerFrame.cpp:3858
(Diff revision 1)
>  
>    for (FlexLine* line = lines.getFirst(); line; line = line->getNext()) {
>  
>      // Main-Axis Alignment - Flexbox spec section 9.5
>      // ==============================================
> -    auto justifyContent = aReflowState.mStylePosition->ComputedJustifyContent();
> +    auto justifyContent = IsLegacyBox(aReflowState.mStyleDisplay) ?

This calculation looks like it's loop-invariant so it's probably worth moving it outside the loop so that we don't do it for each FlexLine. (and make it 'const')

::: layout/style/nsCSSPropAliasList.h:329
(Diff revision 1)
>                 WEBKIT_PREFIX_PREF)
>  
> -// Alias old flexbox properties to modern flexbox pseudo-equivalents:
> +// Alias -webkit-box properties to their -moz-box equivalents.
> +// (NOTE: Even though they're aliases, in practice these -webkit properties
> +// will behave a bit differently from their -moz versions, because they'll be
> +// accompanied by "display:-webkit-box" instead of "display:-moz-box", and we

"because they'll be accompanied by "display:-webkit-box"" is a bit strong since we don't actually know or enforce that.

Perhaps you could rephrase it so that it says "*if* they're accompanied ..." they will behave differently.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8737403 [details]
MozReview Request: Bug 1257688 part 2: Enable "-webkit-box-pack: justify" sections of -webkit-box reftests, & fix reference cases to use 'space-between' (the modern equivalent). (no review)

https://reviewboard.mozilla.org/r/43955/#review40555

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8737407 [details]
MozReview Request: Bug 1257688 part 0: Add an "IsLegacyBox" accessor to nsFlexContainerFrame, to enable special handling of display:-webkit-box & display:-webkit-inline-box. r=mats

https://reviewboard.mozilla.org/r/43957/#review40557

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8737408 [details]
MozReview Request: Bug 1257688 part 3: Change "-webkit-box-ordinal-group" to alias its -moz equivalent, & change nsFlexContainerFrame to respect it in a -webkit-box. r=mats

https://reviewboard.mozilla.org/r/43959/#review40559

Comment 16

[Daniel Holbert [:dholbert]]

Thanks for the reviews! I've addressed comment 12's review comments.

Also, I'm splitting the "IsLegacyBox" impl out of Part 1 and into its own patch -- "Part 0" -- because it's not quite as simple as I thought it'd be.

I was assuming that any webkit-flavored nsFlexContainerFrame would have StyleDisplay()->mDisplay == NS_STYLE_DISPLAY_WEBKIT_BOX, but that's not true when "overflow:hidden" is involved!!  In that case, we get an outer scroll frame with the correct "display" value, but the inner scrolled frame (the nsFlexContainerFrame) counterintuitively has "display:block"! This comes from this ua.css line:
> *|*::-moz-scrolled-content [...] {
>   display: block;
http://mxr.mozilla.org/mozilla-central/source/layout/style/res/ua.css?rev=e302d8d2af8b&mark=156-156,160-162#156

Anyway -- to work around this, we can just check our nsFlexContainerFrame for "display:block" (which is quick & cheap), and if we find that, then we can assume we're -moz-scrolled-content and we can defer to our parent frame's display value.  This is what the impl in my "part 0" will do.

This means IsLegacyBox will now need one additional arg (the style context pointer, so we can look up the parent style context & grab its display value). I'll make that change at the callsites in the later patches without re-requesting review, but feel free to post review comments if you have any concerns about these changes.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1257688 part 4: Add reftests for -webkit-box-ordinal-group inside of display:-webkit-box. r=mats

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

Comment 24

[Daniel Holbert [:dholbert]]

(I filed bug 1261942 on the mozreview weirdness mentioned in comment 23, BTW.)

Current "actual" status here is:
 - Parts 0 and 5 need review.
 - I'll be attaching one more patch (part 6) with reftests for -webkit-box-flex (the feature in part 5) which I may also request review on.

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 8737407 [details]
MozReview Request: Bug 1257688 part 0: Add an "IsLegacyBox" accessor to nsFlexContainerFrame, to enable special handling of display:-webkit-box & display:-webkit-inline-box. r=mats

https://reviewboard.mozilla.org/r/43957/#review40821

Yeah, MozReview is confused, it says this part has r+ already.

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8737918 [details]
MozReview Request: Bug 1257688 part 5: Change "-webkit-box-flex" to alias its -moz equivalent, & change nsFlexContainerFrame to use it instead of flex-shrink & flex-grow in a -webkit-box. r=mats

https://reviewboard.mozilla.org/r/44181/#review40823

::: layout/generic/nsFlexContainerFrame.cpp:1178
(Diff revision 1)
> +  if (IsLegacyBox(aParentReflowState.mStyleDisplay,
> +                  mStyleContext)) {

fits on the same line?

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #26)
> > +  if (IsLegacyBox(aParentReflowState.mStyleDisplay,
> > +                  mStyleContext)) {
> 
> fits on the same line?

It does indeed -- thanks, fixed locally.

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1257688 part 6: Add reftest for -webkit-box-flex inside of display:-webkit-box. r?mats

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

Comment 36

[Daniel Holbert [:dholbert]]

(sorry for review-spam. That was the last patch.)

Part 6's testcase & reference case in part 6 are identical to each other, except for:
 1) <title>
 2) display:-webkit-box vs display:flex
 3) -webkit-box-flex: [number] vs. flex: [number] [number] auto.
   ("[number] [number] auto means "use this number for my flex-grow & flex-shrink, and use the width property to figure out the flex basis)

 4) An extra rule in the reference case to suppress the modern-flexbox "flex-shrink: 1" shrinkable-by-default behavior. (This lets us more effectively be a reference for the testcase, which has no such shrinkable-by-default behavior.)

Comment 37

[Mats Palmgren (inactive)]

Comment on attachment 8737961 [details]
MozReview Request: Bug 1257688 part 6: Add reftest for -webkit-box-flex inside of display:-webkit-box. r?mats

https://reviewboard.mozilla.org/r/44209/#review40871

Comment 38

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71104bf896f8

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3bd2e9a019
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a79edf76f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3daa360f1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5648437459
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5dd5f3f2f39
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c66c105ff25
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d62fbf0a215

Comment 40

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> And I'll spin off a followup bug for "-webkit-box-orient"

Spun off bug 1262049 for this.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d3bd2e9a019
https://hg.mozilla.org/mozilla-central/rev/a9a79edf76f0
https://hg.mozilla.org/mozilla-central/rev/bd3daa360f1b
https://hg.mozilla.org/mozilla-central/rev/fe5648437459
https://hg.mozilla.org/mozilla-central/rev/a5dd5f3f2f39
https://hg.mozilla.org/mozilla-central/rev/3c66c105ff25
https://hg.mozilla.org/mozilla-central/rev/8d62fbf0a215

Comment 45

[Lawrence Mandel [:lmandel] (use needinfo)]

This change looks somewhat more complex than what we typically want to uplift to Beta. However, dup bug 1256664 shows that we're about this ship a regression to Yahoo News in 47 that relies on this change. What options do we have to avoid regressing Yahoo News in 47?

Comment 46

[Mike Taylor [:miketaylr]]

That regression for Yahoo Mail's news feed only reproduces with layout.css.prefixes.webkit=true, which for now is gated in Aurora + Nightly.

(I just verified on a test account: https://cloudup.com/c358euR6JNl, left is 47.0b1, which has layout.css.prefixes.webkit=false by default, right has the regression when I flip layout.css.prefixes.webkit=true)

Comment 47

[Sebastian Zartner [:sebo]]

Marking this as dev-doc-complete as of the discussion started at bug 1208635 comment 17.

Changes made:
Added compatibility notes for this at:

https://developer.mozilla.org/en-US/docs/Web/CSS/box-flex
https://developer.mozilla.org/en-US/docs/Web/CSS/box-ordinal-group
https://developer.mozilla.org/en-US/docs/Web/CSS/box-align
https://developer.mozilla.org/en-US/docs/Web/CSS/box-pack

Added a description of the -webkit-box value to https://developer.mozilla.org/en-US/docs/Web/CSS/display#Values and note about the changes at https://developer.mozilla.org/en-US/Firefox/Releases/48.

Sebastian

Comment 48

[Daniel Holbert [:dholbert]]

Just noticed a minor copypaste typo some of this bug's reftests -- some titles incorrectly say they're testing "-webkit-box-orient" when they should say "-webkit-box-ordinal-group", e.g. here:
  https://hg.mozilla.org/mozilla-central/rev/a5dd5f3f2f39#l3.13

I'm landing a (very late-breaking) followup patch for this issue, on inbound, in a few minutes.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d4f22f391c
followup: Fix typo in <title>s in webkit-box-ordinal-group reftests. (no review, test-metadata only)

Comment 50

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/91d4f22f391c