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

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({dev-doc-complete})

Trunk
mozilla48
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(7 attachments)

58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
(Assignee)

Description

3 years ago
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.)
(Assignee)

Updated

3 years ago
Summary: Support -webkit-box alignment properties as their own properties, not as aliases → Support -webkit-box alignment properties as actual properties, not as aliases
(Assignee)

Comment 1

3 years ago
(I'm going to layer this bug on top of my patches for bug 1236400; adding dependency.)
Depends on: 1236400
(Assignee)

Updated

3 years ago
Blocks: 1231682
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
(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.)
(Assignee)

Comment 4

3 years ago
[updating summary per comment 2]
Summary: Support -webkit-box alignment properties as actual properties, not as aliases → Make -webkit-box alignment properties aliases of legacy -moz-box properties (with custom handling in modern flexbox), not aliases of modern alignment properties
(Assignee)

Comment 5

3 years ago
Patches coming shortly.
(Assignee)

Comment 6

3 years ago
(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.)
Blocks: 1256664, 1258733
No longer depends on: 1208635
(Assignee)

Comment 7

3 years ago
Created 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

Review commit: https://reviewboard.mozilla.org/r/43953/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43953/
Attachment #8737402 - Flags: review?(mats)
(Assignee)

Comment 8

3 years ago
Created 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)

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

Comment 9

3 years ago
Created 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

Review commit: https://reviewboard.mozilla.org/r/43957/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43957/
Attachment #8737407 - Flags: review?(mats)
Attachment #8737408 - Flags: review?(mats)
(Assignee)

Comment 10

3 years ago
Created 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

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

Comment 11

3 years ago
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 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.
Attachment #8737402 - Flags: review?(mats) → review+
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
Attachment #8737403 - Flags: review+
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
Attachment #8737407 - Flags: review?(mats) → review+
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
Attachment #8737408 - Flags: review?(mats) → review+
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
Created attachment 8737917 [details]
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/
Attachment #8737407 - Attachment description: 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 → 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
Attachment #8737402 - Attachment description: 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 → 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
Attachment #8737408 - Attachment description: MozReview Request: Bug 1257688 part 4: Add reftests for -webkit-box-ordinal-group inside of display:-webkit-box. r?mats → 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
Attachment #8737917 - Flags: review?(mats)
Attachment #8737918 - Flags: review?(mats)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Updated

3 years ago
Attachment #8737917 - Flags: review?(mats) → review+
(Assignee)

Comment 24

3 years ago
(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 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.
Attachment #8737407 - Flags: review?(mats) → review+

Updated

3 years ago
Attachment #8737918 - Flags: review?(mats) → review+
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?
(Assignee)

Comment 27

3 years ago
(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.
(Assignee)

Comment 28

3 years ago
Created attachment 8737961 [details]
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/
Attachment #8737407 - Attachment description: 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 → 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
Attachment #8737918 - Attachment description: 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 → 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
Attachment #8737961 - Flags: review?(mats)
Attachment #8737917 - Flags: review+
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Comment 36

3 years ago
(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 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
Attachment #8737961 - Flags: review?(mats) → review+
(Assignee)

Updated

3 years ago
Blocks: 1262049
(Assignee)

Comment 40

3 years ago
(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.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1238580
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1256664
(Assignee)

Updated

3 years ago
No longer blocks: 1258733
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1258733
(Assignee)

Updated

3 years ago
No longer blocks: 1256664
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?
Flags: needinfo?(dholbert)
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)
Flags: needinfo?(dholbert)
(Assignee)

Updated

2 years ago
Depends on: 1272721
(Assignee)

Updated

2 years ago
No longer depends on: 1272721
Depends on: 1208635
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
Keywords: dev-doc-complete
(Assignee)

Comment 48

11 months ago
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

11 months ago
Pushed by dholbert@mozilla.com:
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)
You need to log in before you can comment on or make changes to this bug.