Closed Bug 1262049 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 2 obsolete files)

25.11 KB, patch
Details | Diff | Splinter Review
25.66 KB, patch
Details | Diff | Splinter Review
6.36 KB, patch
mats
: review+
Details | Diff | Splinter Review
4.49 KB, patch
mats
: review+
Details | Diff | Splinter Review
2.54 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.73 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.63 KB, patch
Details | Diff | Splinter Review
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.
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
Attachment #8739811 - Attachment description: clean backout of bug 1208344 → clean backout of bug 1208344 (for illustration/comparison purposes)
Attachment #8739811 - Attachment is patch: true
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.)
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.)
Attachment #8743090 - Flags: review?(mats)
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).
Attachment #8743097 - Flags: review?(mats)
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.
Attachment #8743098 - Flags: review?(mats)
(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.)
(Sorry, the previous "part 4" had the wrong patch description in its commit message. Fixed here.)
Attachment #8743098 - Attachment is obsolete: true
Attachment #8743098 - Flags: review?(mats)
Attachment #8743100 - Flags: review?(mats)
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.)
Attachment #8743145 - Flags: review?(mats)
Attachment #8743090 - Attachment is obsolete: true
Attachment #8743090 - Flags: review?(mats)
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.)
Attachment #8743149 - Flags: review?(mats)
(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.)
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.
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
Attachment #8743145 - Flags: review?(mats) → review+
Attachment #8743097 - Flags: review?(mats) → review+
Attachment #8743100 - Flags: review?(mats) → review+
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.
Attachment #8743149 - Flags: review?(mats) → review+
(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!
(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 {
Flags: needinfo?(mats)
That's excellent, thanks!

minor nit: s/these properties/the legacy properties/
Flags: needinfo?(mats)
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
(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.
Blocks: 1266248
Depends on: 1272721
Duplicate of this bug: 1230708
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
Flags: needinfo?(dholbert)
Looks good - thanks!
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.