Closed Bug 1257688 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

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
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
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)
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
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
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
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
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
MozReview Request: Bug 1257688 part 4: Add reftests for -webkit-box-ordinal-group inside of display:-webkit-box. r=mats
58 bytes, text/x-review-board-request
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
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
MozReview Request: Bug 1257688 part 6: Add reftest for -webkit-box-flex inside of display:-webkit-box. r?mats
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
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.)
Summary: Support -webkit-box alignment properties as their own properties, not as aliases → Support -webkit-box alignment properties as actual properties, not as aliases
(I'm going to layer this bug on top of my patches for bug 1236400; adding dependency.)
Depends on: 1236400
Blocks: 1231682
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.
(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.)
[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
Patches coming shortly.
(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
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+
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.
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)
Attachment #8737917 - Flags: review?(mats) → review+
(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+
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?
(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.
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+
(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+
Blocks: 1262049
(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.
No longer blocks: 1258733
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)
Depends on: 1272721
No longer depends on: 1272721
Depends on: 1208635
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: