1262049 - Change -webkit-box-orient to alias -moz-box-orient (and make nsFlexContainerFrame check that property in a -webkit-box)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Spinning this off of bug 1257688 to redo the way we alias the "-webkit-box-orient" property.

BACKSTORY:
Originally, we were trying to alias all the -webkit-box properties directly to their modern equivalents. But mats pointed out some problems with that, for some of the modern properties. So now, we're converting them to alias their much-more-similar "-moz-box" properties, and we're changing nsFlexContainerFrame to respect those properties (instead of the modern ones) for elements with display:-webkit-box.

So: that conversion is mostly done (in bug 1257688), but as noted in bug 1257688 comment 11, I'm spinning off "-webkit-box-orient" to its own bug (this bug here), since our current aliasing strategy for that property is a bit more complex (because its values are different from its modern flexbox equivalent's values).

The fix here will largely involve backing out the patch-stack from bug 1208344.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: clean backout of bug 1208344 (for illustration/comparison purposes)

The first patch here will be a backout of bug 1208344, in its entirety. (There's actually a bit of refactoring/renaming in that bug that we should probably keep, but for simplicity let's just back it all out, and I'll add back anything worth keeping after that.)

This clean backout patch was generated with the following commands:
  hg up -r 0b5ed5e4a395            # final cset of bug 1208344
  hg revert -r 6f4da397ac3c --all  # parent cset of bug 1208344's 1st patch
  hg qnew backout-bug-1208344

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: Bug 1262049 part 1: Back out bug 1208344 in its entirety, and mark -webkit-box-orient:vertical reftests as failing (for now)

Here's that same backout, unbitrotted to apply on trunk. (Most of the unbitrotting was as a result of whitespace changes in affected areas of code.)

I've also marked two reftests as "fails" for the moment, because they depend on the backed-out feature (-webkit-box-orient:vertical).  (They'll go back to passing with later patches in this bug.)

I'm not going to bother with review on this patch, since it's just a backout + 2 reftest annotations, and I've convinced myself that my unbitrotting was correct.  Anyone can feel free to check my work, though (e.g. feel free to compare the previous patch against this one, in a merge tool like "meld", which will mostly figure out whitespace-only changes & will just highlight the differing whitespace on those lines.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 2: add back -webkit-box-orient as an alias for -moz-box-orient

After "part 1" removes our existing (hacky) support for -webkit-box-orient, this patch adds it back as an alias for -moz-box-orient.

(It's not yet respected by nsFlexContainerFrame, though, so it won't have any effect in a display:-webkit-box element yet.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Adjust FlexboxAxisTracker constructor to take pointer to nsFlexContainerFrame instead of style struct

What follows will be a series of patches that adjust FlexboxAxisTracker to support "legacy" properties vs. "modern" properties.

(FlexboxAxisTracker is the class that determines the directions of our main axis & cross axis.)

This first patch makes FlexboxAxisTracker's constructor take a pointer to the flex frame, instead a pointer to its nsStylePosition (because it'll need to be able to access other style structs beyond just that one, to look up legacy -webkit-box properties).

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 4: Split out "InitAxesFromModernProps" helper

This patch splits out some logic from the FlexboxAxisTracker constructor into a helper function, "InitAxesFromModernProps".

(The patch technically represents the change with that logic staying in-place, with a function-signature inserted before it, and the end of the constructor (" // Master switch [...]") moving upwards to before the new function name.)

This patch is idempotent -- just refactoring out some code into a helper.

Comment 6

[Daniel Holbert [:dholbert]]

(I have another WIP patch that actually adds the legacy stuff; will post that soon, probably later tonight. Wanted to get the earlier easy patches up for review, though, since they're ready.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 4 v2: Split out "InitAxesFromModernProps" helper

(Sorry, the previous "part 4" had the wrong patch description in its commit message. Fixed here.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 2, v2: Add -webkit-box-orient & -webkit-box-direction as aliases for -moz versions

Actually, I realized I should add "-webkit-box-direction" as well, for completeness. (It fits into the new FlexboxAxisTracker initialization code that I'll be adding in part 5 pretty cleanly.)

So, here's an updated "part 2" that adds that property as well.

(This property -- -webkit-box-direction -- was the only -webkit-box-* property that we haven't supported up until now, because there's no good way to map it to a modern flexbox property in the style system, and we hadn't run across any sites that depend on it. But, now that we're going with a different strategy for these properties (mapping them to their -moz equivalents & checking those in modern flexbox code), it's easy to add, so we might as well support it for completeness.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: part 5: Add "InitAxesFromLegacyProps" (to honor -webkit-box-orient & -webkit-box-direction when determining axes for a -webkit-box flexbox)

This patch creates a new function like the one spun out in part 4, to respect legacy properties instead of modern properties for determining the axes.

(As flagged with XXXdholbert comments in the patch, mMainAxis and mCrossAxis are cruft that I still need to finish removing as part of the last pieces of meta bug 1079155. I'm setting them in this Init method right now, but ultimately they'll be removed. mIsRowOriented, mIsMainAxisReversed, and mIsCrossAxisReversed are the member-variables that really matter here, going forward.)

Comment 10

[Daniel Holbert [:dholbert]]

(Note that part 5 reenables the reftests that I marked as failing in part 1 (noted in comment 2), since they pass again, now that we're respecting -webkit-box-orient in layout again.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: part 6: add reftest for -webkit-box-direction

Here's a final patch, which adds a reftest for -webkit-box-direction. (And -webkit-box-orient is already implicitly reftested by other tests -- the ones that I disabled/re-enabled in earlier patches here)

Not bothering with review for this reftest, but feedback is welcome.

Comment 12

[Daniel Holbert [:dholbert]]

As something approaching spec reference text, here are the MDN pages about the "-moz" versions of these properties, BTW (which are applicable to the -webkit versions as well, at least in terms of accepted values & the spirit of what they do):
  https://developer.mozilla.org/en-US/docs/Web/CSS/box-orient
  https://developer.mozilla.org/en-US/docs/Web/CSS/box-direction

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8743149 [details] [diff] [review]
part 5: Add "InitAxesFromLegacyProps" (to honor -webkit-box-orient & -webkit-box-direction when determining axes for a -webkit-box flexbox)

Nits:

>+  // Helpers for constructor which determine the orientation of our axes, based
>+  // on legacy box properties (-webkit-box-orient, -webkit-box-direction) or

That includes -moz- variants too, right?

It would be good to have a doc comment on the class in the .h  that
gives a short overview how -webkit-* are aliased to -moz-* and that
the class implements different behavior depending on its 'display' value.

It's a bit tedious to write out -[moz|webkit]-box-orient everywhere
though, so perhaps just mentioning one is fine if one can assume
it stands for [moz|webkit] everywhere without exceptions.
Is -*-box-orient too obscure?

>+  // modern flexbox properties (flex-direction, flex-wrap) depending on whether

Swap the order of the properties so that they "match" the legacy names?

>+  if (styleXUL->mBoxDirection ==
>+      NS_STYLE_BOX_DIRECTION_REVERSE) {

Looks like it would fit on one line.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> >+  // Helpers for constructor which determine the orientation of our axes, based
> >+  // on legacy box properties (-webkit-box-orient, -webkit-box-direction) or
> 
> That includes -moz- variants too, right?

(Technically yes, but only inside of a "display:-webkit-box" -- not inside of a "display:-moz-box".)

> It would be good to have a doc comment on the class in the .h  that
> gives a short overview how -webkit-* are aliased to -moz-* and that
> the class implements different behavior depending on its 'display' value.

In nsFlexContainerFrame.h? Makes sense -- sure, I'll add something like that.

> It's a bit tedious to write out -[moz|webkit]-box-orient everywhere
> though, so perhaps just mentioning one is fine if one can assume
> it stands for [moz|webkit] everywhere without exceptions.
> Is -*-box-orient too obscure?

I think that's perhaps too obscure... Realistically, we're only ever expecting to be exercising this code with "-webkit" styles (because they have to be inside of a display:-webkit-box), so I tend to think we should just use those consistently, with a single clarifying comment explaining the aliasing nuance (as you suggested in the .h file).

(If we hint at -moz styles having influence here, it gives the impression that -moz-box can produce a nsFlexContainerFrame, which it cannot. I'd rather just hand-wave in short comments with the expected use-case, and then dig into the deeper details in one single spot.)

> >+  // modern flexbox properties (flex-direction, flex-wrap) depending on whether
> 
> Swap the order of the properties so that they "match" the legacy names?

Mmm, I see what you mean. But, I'd like to leave these as-is, if it's all right with you. Despite the property-naming, the legacy vs. modern "direction" properties don't actually match, from an importance perspective.

The current ordering makes more sense to me, because in each case the first listed property (-webkit-box-orient, flex-direction) is the primary determination of the axes, and the second listed property (-webkit-box-direction, flex-wrap) is an afterthought that adds some nuance (possibly flipping the polarity of one axis or the other).

> >+  if (styleXUL->mBoxDirection ==
> >+      NS_STYLE_BOX_DIRECTION_REVERSE) {
> 
> Looks like it would fit on one line.

I'll collapse it -- thanks!

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> (In reply to Mats Palmgren (:mats) from comment #13)
> > It would be good to have a doc comment on the class in the .h  that
> > gives a short overview how -webkit-* are aliased to -moz-* and that
> > the class implements different behavior depending on its 'display' value.
> 
> In nsFlexContainerFrame.h? Makes sense -- sure, I'll add something like that.

Here's the comment that I'm tentatively going with in nsFlexContainer.h, just before "class nsFlexContainer {".  Let me know if you have any feedback:
=====

/**
 * This is the rendering object used for laying out elements with
 * "display: flex" or "display: inline-flex".
 *
 * We also use this class for elements with "display: -webkit-box" or
 * "display: -webkit-inline-box" (but not "-moz-box" / "-moz-inline-box" --
 * those are rendered with old-school XUL frame classes).
 *
 * Note: we represent the -webkit-box family of properties (-webkit-box-orient,
 * -webkit-box-flex, etc.) as aliases for their -moz equivalents.  And for
 * -webkit-{inline-}box containers, nsFlexContainerFrame will honor those
 * legacy for alignment/flexibility/etc. *instead of* honoring the modern
 * flexbox & alignment properties.  For brevity, many comments in
 * nsFlexContainerFrame.cpp simply refer to these properties using their
 * "-webkit" versions, since we're mostly expecting to encounter them in that
 * form. (Technically, the "-moz" versions of these properties *can*
 * influence layout here as well (since that's what the -webkit versions are
 * aliased to) -- but only inside of a "display:-webkit-{inline-}box"
 * container.)
 */
class nsFlexContainerFrame : public nsContainerFrame {

Comment 16

[Mats Palmgren (inactive)]

That's excellent, thanks!

minor nit: s/these properties/the legacy properties/

Comment 17

[Daniel Holbert [:dholbert]]

Thanks! I've fixed that typo.

I'll land shortly -- here's a Try run from yesterday, which was successful aside from some unrelated intermittent oranges: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03f933614c65&selectedJob=19713808

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> The first patch here will be a backout of bug 1208344, in its entirety.
> (There's actually a bit of refactoring/renaming in that bug that we should
> probably keep, but for simplicity let's just back it all out, and I'll add
> back anything worth keeping after that.)

Note: I'll spin off a followup to re-land the trivial, still-useful pieces of bug 1208344 that I hinted at here.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f41c81e14d2

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d701138d43e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d6255dffb0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5b422dbad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/020c4ce999d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c402a683b04f

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d701138d43e9
https://hg.mozilla.org/mozilla-central/rev/9d6255dffb0b
https://hg.mozilla.org/mozilla-central/rev/fa5b422dbad8
https://hg.mozilla.org/mozilla-central/rev/020c4ce999d9
https://hg.mozilla.org/mozilla-central/rev/c402a683b04f

Comment 23

[Sebastian Zartner [:sebo]]

Added compatibility notes to https://developer.mozilla.org/en-US/docs/Web/CSS/box-direction and https://developer.mozilla.org/en-US/docs/Web/CSS/box-orient and listed the two new properties at https://developer.mozilla.org/en-US/Firefox/Releases/48.

Daniel, please have a quick look if the notes are ok.

Sebastian

Comment 24

[Daniel Holbert [:dholbert]]

Looks good - thanks!