1174003 - Logicalize IsCrossAxisHorizontal()/IsMainAxisHorizontal() checks in flexbox layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

We've got some IsCrossAxisHorizontal() & IsMainAxisHorizontal() checks in flexbox layout code which need to be logicalized in order to fully support vertical writing-modes.

Comment 1

[Daniel Holbert [:dholbert]]

I've got a patch stack for this that I'll be posting shortly.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 1: [css-flexbox] Remove unused method nsFlexContainerFrame::IsHorizontal.

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

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 2: [css-flexbox] Reformat code around GetMinimumWidgetSize call slightly.

This patch doesn't change behavior. It's purely to allow the next patch to be more surgical. Specifically, this patch:
 - splits a subtract-and-clamp operation into two separate operations.
 - splits one a comment into two.
...so that the next patch can swap out these variables for new ones, without
pushing these lines over 80 characters.

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

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 3: [css-flexbox] Make GetMarginSizeInMainAxis() take a LogicalMargin, instead of nsMargin.

This patch doesn't change behavior. It just makes us use logical axes/types instead of physical ones for this particular API and its caller.

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

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 4: [css-flexbox] Remove IsMainAxisHorizontal() check from DoFlexLayout().

This patch doesn't affect behavior.  It just replaces some (correct) physical-axis-dependent code with equivalent logical-axis-dependent code.

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

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 5: [css-flexbox] Remove is-{main,cross}-axis-horizontal checks from ReflowFlexItem.

This patch mostly[1] doesn't affect behavior.  It just changes some ReflowInput
width/height-setting logic to use ISize/BSize setters instead of width/height
setters.

To help with this, I'm also adding some more getters to answer the question "is
this flex item's {inline,block} axis the same as the flex container's
{main,cross} axis", for convenience/readability at callsites. (All of these
getters are simply giving a different view of the same underlying single bit of
information.)

[1] One way this patch *kind of* affects behavior: it generalizes the
conditions under which we set the NS_FRAME_CONTAINS_RELATIVE_BSIZE flag on a
flex item.  But that doesn't actually have any user-visible effects right now,
because that flag's only purpose is to determine whether we should reflow, and
currently we always reflow all flex items. If/when that changes, though
(i.e. when we start reflowing flex items more selectively), this patch is
adding a reftest that may test this generalized behavior. (The reftest actually
trivially passes right now, due to unrelated bug 1441348.)

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 6: [css-flexbox] Replace ComputedCrossSize() helper with a new API that uses logical axes internally.

This patch doesn't affect behavior.

It removes a helper-function that simply returned nsStylePosition::mWidth or
mHeight -- whichever was in the flex container's cross axis. This helper was
only used to answer the question "is the cross size 'auto'", at a single
callsite.  So, this patch replaces the helper with a new helper that more
directly answers that question.  The new helper's implementation uses logical
axes for its reasoning, too, whereas the removed one used physical axes (and in
particular, it relied on AxisOrientationTracker::IsCrossAxisHorizontal(), which
I'll be getting rid of later in this patch series).

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

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 7: [css-flexbox] Logicalize IsCrossAxisHorizontal() check in GetBaselineOffsetFromOuterCrossSize (and simplify a condition for baseline fallback).

This patch doesn't change our behavior -- not in opt builds, at least. In debug
builds, this patch does change an assertion condition, to remove a usage of
IsCrossAxisHorizontal().  This means debug builds may proceed a bit further
when loading e.g. bug 1384266's testcase (which up until now was tripping this
assertion).  Now that testcase fails a slighlty later assertion (which I'll
sort out on that bug).

The first hunk of this patch is just a simplification -- replacing a
complicated condition (IsRowOriented==IsOrthogonalTo) with a simpler
formulation of the same condition.  I'm making that simplification in this
patch so that we're more clearly consistent about what condition we depend on
for baseline alignment.  After this patch, that (simplified) condition matches
the condition that we assert inside of GetBaselineOffsetFromOuterCrossEdge().

Note: I'm adding two XXXdholbert comments in this patch, for outstanding issues
that I ran across which are out-of-scope for this patch series.

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

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 8: [css-flexbox] Change flex item intrinsic ratio calculations to use logical axes and a LogicalSize.

This patch doesn't affect behavior. It does the following:
 - Changes the AxisOrientationTracker "GetMainComponent/GetCrossComponent" APIs
   to take a LogicalSize rather than a nsSize.
 - Changes FlexItem::mIntrinsicRatio to be a LogicalSize rather than a nsSize.
 - Simplifies the MainSizeFromAspectRatio() helper-function (in particular, it
   removes a call to IsCrossAxisHorizontal(), which is an API I'm gradually
   removing in this patch series.)

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

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 9: [css-flexbox] Remove GET_MAIN_COMPONENT calls from CheckForMinSizeAuto().

This patch doesn't change behavior.

The GET_MAIN_COMPONENT macro (some of whose calls I'm removing here) makes a
call to IsMainAxisHorizontal() under the hood.  So I want to get rid of calls to this this macro, to get closer to killing that method.

In this code, we're interested in the flex item's min-size property in the flex
container's main axis.  This patch makes us simply use MinISize/MinBSize (in
terms of the *flex container's* writing mode) to get the appropriate min-size
property.  The call to IsRowOriented() (querying the flex container's
"flex-direction" property) tells us whether the inline or block axis is the
main axis.

This patch also does away with an unnecessary axis-specific 'overflow-{x/y}'
check, which we don't need to bother with, as noted in a new code-comment (due
to how the 'overflow' subproperties influence each other).

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

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 10: [css-flexbox] Remove GET_MAIN_COMPONENT/GET_CROSS_COMPONENT macros (expanding each at its only remaining callsite).

This patch doesn't affect behavior. Each of these macros only had one caller
remaining, and this patch simply expands each of these macros at its sole
callsite.

Note that I'm using the "IsMainAxisHorizontal()" helper in *both* expansions
here, which makes the alternative variant "IsCrossAxisHorizontal()" unused as
of this patch. The next patch in this series will remove that now-unused
method, which will reduce the "Horizontal"-querying API surface here.

The documentation for the macros is still relevant to their still-existing
"_LOGICAL" variants (and those variants do still have callers), so I'm updating
the documentation to be about those variants.

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

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Bug 1174003 part 11: [css-flexbox] Remove IsCrossAxisHorizontal(), and make IsMainAxisHorizontal() a private implementation detail.

At this point in the series:
 - AxisOrientationTracker::IsCrossAxisHorizontal() has zero callers, so it can
   be deleted.
 - AxisOrientationTracker::IsMainAxisHorizontal() only has two callers, and
   both are inside its own class. So it's effectively become an implementation
   detail of its class, and it can be made private.  (I'm not entirely removing
   it, because it does make its two callers more readable. The callers are
   working with a physical-axis-dependent type, 'LayoutDeviceIntSize', which
   comes from an API that isn't logical-axis-friendly, "GetMinimumWidgetSize",
   so they're not easily logicalized. So: it's nice to keep
   IsMainAxisHorizontal() around as an internal convenience method to tell us
   whether to extract the width or height inside of these two specific
   methods. But we don't want to introduce more callers, so let's leave it
   around & make it private.)

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

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8954608 [details]
Bug 1174003 part 1: [css-flexbox] Remove unused method nsFlexContainerFrame::IsHorizontal.

https://reviewboard.mozilla.org/r/223636/#review229754

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8954609 [details]
Bug 1174003 part 2: [css-flexbox] Reformat code around GetMinimumWidgetSize call slightly.

https://reviewboard.mozilla.org/r/223638/#review229756

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8954610 [details]
Bug 1174003 part 3: [css-flexbox] Make GetMarginSizeInMainAxis() take a LogicalMargin, instead of nsMargin.

https://reviewboard.mozilla.org/r/223640/#review229758

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8954611 [details]
Bug 1174003 part 4: [css-flexbox] Remove IsMainAxisHorizontal() check from DoFlexLayout().

https://reviewboard.mozilla.org/r/223642/#review229836

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 8954612 [details]
Bug 1174003 part 5: [css-flexbox] Remove is-{main,cross}-axis-horizontal checks from ReflowFlexItem.

https://reviewboard.mozilla.org/r/223644/#review229838

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8954613 [details]
Bug 1174003 part 6: [css-flexbox] Replace ComputedCrossSize() helper with a new API that uses logical axes internally.

https://reviewboard.mozilla.org/r/223646/#review229840

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8954614 [details]
Bug 1174003 part 7: [css-flexbox] Logicalize IsCrossAxisHorizontal() check in GetBaselineOffsetFromOuterCrossSize (and simplify a condition for baseline fallback).

https://reviewboard.mozilla.org/r/223648/#review229966

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8954615 [details]
Bug 1174003 part 8: [css-flexbox] Change flex item intrinsic ratio calculations to use logical axes and a LogicalSize.

https://reviewboard.mozilla.org/r/223650/#review229970

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8954616 [details]
Bug 1174003 part 9: [css-flexbox] Remove GET_MAIN_COMPONENT calls from CheckForMinSizeAuto().

https://reviewboard.mozilla.org/r/223652/#review229972

::: commit-message-69780:6
(Diff revision 1)
> +call to IsMainAxisHorizontal() under the hood.  So I want to get rid of calls to this this macro, to get closer to killing that method.
> +

You probably intended to do a line break somewhere around "calls"?

::: layout/generic/nsFlexContainerFrame.cpp:1941
(Diff revision 1)
>    const nsStylePosition* pos = aFlexItemReflowInput.mStylePosition;
>    const nsStyleDisplay* disp = aFlexItemReflowInput.mStyleDisplay;

Out-of-scope for this patch series, but this is probably wrong for tables, right?  (I suspect you need the same fix I did in Grid)

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 8954617 [details]
Bug 1174003 part 10: [css-flexbox] Remove GET_MAIN_COMPONENT/GET_CROSS_COMPONENT macros (expanding each at its only remaining callsite).

https://reviewboard.mozilla.org/r/223654/#review229976

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8954618 [details]
Bug 1174003 part 11: [css-flexbox] Remove IsCrossAxisHorizontal(), and make IsMainAxisHorizontal() a private implementation detail.

https://reviewboard.mozilla.org/r/223656/#review229980

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8954616 [details]
Bug 1174003 part 9: [css-flexbox] Remove GET_MAIN_COMPONENT calls from CheckForMinSizeAuto().

https://reviewboard.mozilla.org/r/223652/#review229972

> You probably intended to do a line break somewhere around "calls"?

Indeed, thanks.

> Out-of-scope for this patch series, but this is probably wrong for tables, right?  (I suspect you need the same fix I did in Grid)

I added a note about this over on bug 1306403 (which is about tables as flex items & various properties making it through the wrapper one way or another).

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8954616 [details]
Bug 1174003 part 9: [css-flexbox] Remove GET_MAIN_COMPONENT calls from CheckForMinSizeAuto().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223652/diff/1-2/

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8954617 [details]
Bug 1174003 part 10: [css-flexbox] Remove GET_MAIN_COMPONENT/GET_CROSS_COMPONENT macros (expanding each at its only remaining callsite).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223654/diff/1-2/

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8954618 [details]
Bug 1174003 part 11: [css-flexbox] Remove IsCrossAxisHorizontal(), and make IsMainAxisHorizontal() a private implementation detail.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223656/diff/1-2/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cacbcc051b29
part 1: [css-flexbox] Remove unused method nsFlexContainerFrame::IsHorizontal. r=mats
https://hg.mozilla.org/integration/autoland/rev/0d797523af03
part 2: [css-flexbox] Reformat code around GetMinimumWidgetSize call slightly. r=mats
https://hg.mozilla.org/integration/autoland/rev/934b53dafa35
part 3: [css-flexbox] Make GetMarginSizeInMainAxis() take a LogicalMargin, instead of nsMargin. r=mats
https://hg.mozilla.org/integration/autoland/rev/201aadc96164
part 4: [css-flexbox] Remove IsMainAxisHorizontal() check from DoFlexLayout(). r=mats
https://hg.mozilla.org/integration/autoland/rev/0653058864df
part 5: [css-flexbox] Remove is-{main,cross}-axis-horizontal checks from ReflowFlexItem. r=mats
https://hg.mozilla.org/integration/autoland/rev/88d72b7833f5
part 6: [css-flexbox] Replace ComputedCrossSize() helper with a new API that uses logical axes internally. r=mats
https://hg.mozilla.org/integration/autoland/rev/af2524d9b0ab
part 7: [css-flexbox] Logicalize IsCrossAxisHorizontal() check in GetBaselineOffsetFromOuterCrossSize (and simplify a condition for baseline fallback). r=mats
https://hg.mozilla.org/integration/autoland/rev/dd08d793fa67
part 8: [css-flexbox] Change flex item intrinsic ratio calculations to use logical axes and a LogicalSize. r=mats
https://hg.mozilla.org/integration/autoland/rev/ec67f4f9bb89
part 9: [css-flexbox] Remove GET_MAIN_COMPONENT calls from CheckForMinSizeAuto(). r=mats
https://hg.mozilla.org/integration/autoland/rev/d0c4313e6d22
part 10: [css-flexbox] Remove GET_MAIN_COMPONENT/GET_CROSS_COMPONENT macros (expanding each at its only remaining callsite). r=mats
https://hg.mozilla.org/integration/autoland/rev/c723ca78deaa
part 11: [css-flexbox] Remove IsCrossAxisHorizontal(), and make IsMainAxisHorizontal() a private implementation detail. r=mats

Comment 29

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/cacbcc051b29
https://hg.mozilla.org/mozilla-central/rev/0d797523af03
https://hg.mozilla.org/mozilla-central/rev/934b53dafa35
https://hg.mozilla.org/mozilla-central/rev/201aadc96164
https://hg.mozilla.org/mozilla-central/rev/0653058864df
https://hg.mozilla.org/mozilla-central/rev/88d72b7833f5
https://hg.mozilla.org/mozilla-central/rev/af2524d9b0ab
https://hg.mozilla.org/mozilla-central/rev/dd08d793fa67
https://hg.mozilla.org/mozilla-central/rev/ec67f4f9bb89
https://hg.mozilla.org/mozilla-central/rev/d0c4313e6d22
https://hg.mozilla.org/mozilla-central/rev/c723ca78deaa

Comment 30

[Daniel Holbert [:dholbert]]

Note: in part 7, I added a XXXdholbert comment about what 'align-self:baseline' falls back to when it's not applicable:
https://hg.mozilla.org/mozilla-central/rev/af2524d9b0ab#l1.23

That spec text has now been clarified such that our code is correct on this point, in this github issue:
 https://github.com/w3c/csswg-drafts/issues/2316

So: I'll push a followup patch here to drop this code comment.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c6c2038fc7
late-breaking followup: remove obsolete comment about flex align-self:baseline behavior. (no review, comment-only, DONTBUILD)

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/34c6c2038fc7