1152913 - Make FlexboxAxisTracker & PositionTracker classes use logical axes instead of physical axes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug on converting FlexboxAxisTracker and the various PositionTracker classes to use logical axes.

Quick recap of what these classes do:
=====================================
* FlexboxAxisTracker class:
   - Stores a representation of our main & cross axes.
     (currently using a physical axis enum, e.g. "eAxis_LR" for left-to-right.)
   - Has utility functions to e.g. grab the main-axis component off of a nsSize.


* PositionTracker classes:
  - Manage our position in a given axis, as we advance across flex items & spacing between them.
  - Stores a representation of the axis, and uses that to figure out when to actually update the internal position (e.g. in the LTR direction, the PositionTracker advances across a frame's size *after* we've placed the frame; but in the RTL direction, the PositionTracker advances across a frame's size *before* we've placed the frame. This is abstracted into the EnterChildFrame/ExitChildFrame methods).

Planned changes:
================
  (A) FlexboxAxisTracker will change so that, instead of storing physical axes, it'll store our WritingMode (which encapsulates our logical axes) and a few bools to represent how our flex axes map to the logical axes.  (Together, these let us *reconstruct* the physical axes that our flex axes map to, if & when we need them.)

  (B) The PositionTracker classes will change so that, instead of storing physical axes, they simply store a bool representing whether the tracked flex axis is parallel or anti-parallel to the corresponding logical axis. (This lets us know when we should advance across frame-sizes -- before or after we've placed the frame -- like the LTR vs. RTL scenario described above with physical coordinates.)

  (C) We'll need to change how we interpret the positions computed by PositionTracker to be in terms of our logical axes, too, or else part (B) will break behavior in e.g. RTL writing-modes.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Make FlexboxAxisTracker track logical axes & how flex axes map to them

This adds member-vars to FlexboxAxisTracker (and accessors for them), to track the logical axes (via the WritingMode) and to track how our flex axes map onto them.

This patch doesn't *use* these values yet; it just sets them up. Their setup is pretty straightforward:
 - "flex-direction: row*" cause mIsRowOriented to be true
 - "flex-direction: *-reverse" cause our main axis to be reversed with respect to our writing-mode, so these values trigger mMainAxisPolarityMatchesWM = false (else, it's true)
 - "flex-wrap: wrap-reverse" cause our cross axis to be reversed with respect to our writing-mode, so this value triggers mCrossAxisPolarityMatchesWM to be false (else, it's true)

Comment 2

[Daniel Holbert [:dholbert]]

(Part 1 means we can get rid of physical-axis-representaitons mMainAxis and mCrossAxis and their accessors, once their callers/usages have all been converted. I've marked them as deprecated for now, and I plan on removing them in a later patch here.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1, v2: Make FlexboxAxisTracker track logical axes & how flex axes map to them

(rebased part 1 on top of trivial bug 1152951)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: Convert 'IsAxisHorizontal()' checks to use new logical-axis member data.

With the information from part 1, we can now remove the last callers of "IsAxisHorizontal" left behind by bug 1148298, and remove that method altogether.

This method does that -- reimplementing Is[Main|Cross]AxisHorizontal with the new member-variables that we're tracking as of "part 1" here.

(I'm also removing an assertion about IsMainAxisHorizontal() != IsCrossAxisHorizontal(), since that assertion is now trivially satisfied, now that one of those methods is implemented in terms of the other.)

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8590494 [details] [diff] [review]
part 1, v2: Make FlexboxAxisTracker track logical axes & how flex axes map to them

> (e.g. LTR & RTL are opposite polarity.)

I don't understand why you call this "polarity".
Isn't it just the same as "direction"?

If so, then I'd prefer "direction" since that's a concept that
everyone is already familiar with.

> DoesMainAxisPolarityMatchWM

Bike-shedding a bit on that using "direction" instead:
MainAxisDirectionEqualsWM
MainAxisDirectionMatchesWM
MainAxisMatchesWMDirection
IsMainAxisInWMDirection
MainAxisIsInWMDirection

Anyone of those would be clearer to the casual reader, IMO.

Also, I'd prefer the bool members to be named the same as the accessor
prefixed with an "m" (modulo any "Is" "Has" "Does" prefix).  This makes
it easier to find all relevant code in an editor, DXR etc.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #5)
> I don't understand why you call this "polarity".
> Isn't it just the same as "direction"?

In my mind, "polarity" is slightly clearer, because it has more of a single-dimensional flavor.  The alternative naming e.g. "IsMainAxisInWMDirection" sounds a bit to me like it tells you whether the main-axis is horizontal or vertical, when really I want it to tell you which flavor of horizontal we are (or which flavor of vertical).

Really, I should probably call this "mIsMainAxisReversed" and "mIsCrossAxisReversed", since these come from flex-direction:[row|column]-reverse & flex-wrap: wrap-reverse, and that's much more pretty readable/understandable.

(I was shying away from "reversed" to avoid confusion with "mAreAxesInternallyReversed", but I could rename that variable to use "flipped" or something, or maybe it doesn't matter with the documentation that I added.)

> Also, I'd prefer the bool members to be named the same as the accessor
> prefixed with an "m" (modulo any "Is" "Has" "Does" prefix).  This makes
> it easier to find all relevant code in an editor, DXR etc.

Makes sense, yup. I'll fix that.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Really, I should probably call this "mIsMainAxisReversed"

Yeah, that's concrete and easy to understand.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 1, v3: Make FlexboxAxisTracker track logical axes & how flex axes map to them

Here's the updated version of part 1, with mMainAxisPolarityMatchesWM converted to mIsMainAxisReversed (and the logic flipped) per comment 7.

Carrying forward r+.

Comment 9

[Daniel Holbert [:dholbert]]

Landed part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8c7869b1a8
   and part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/aba83736383f

(more parts coming; adding leave-open keyword)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe8c7869b1a8
https://hg.mozilla.org/mozilla-central/rev/aba83736383f

Comment 11

[Daniel Holbert [:dholbert]]

Er, looks like I didn't actually add 'leave-open', as I meant to in comment 9. :)  Reopening.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: part 3a: Make PositionTracker classes depend on whether axis is reversed, instead of whether axis is in physically positive direction

"part 3" is about getting Flex Items position & size in logical coordinates.
I'm splitting it into two sub-parts, for better understandability (though I'll merge them when landing, to avoid breaking tests):

 - part 3a makes each PositionTracker class track whether it's *reversed with respect to the corresponding logical axis*, instead of tracking whether its *physical axis grows in a positive direction*.  This information is used for making the decision about when to actually advance across a frame's size, as hinted at in the second bullet-point under "PositionTracker classes" in comment 0.

(This leaves PositionTracker::mAxis mostly -- but not entirely -- unused. These classes still use it for e.g. looking up the margin/border/padding in that axis. A later patch that logicalizes margin-handling will let us remove mAxis. For now, I'm marking it as deprecated.)

 - part 3b (coming up next) will convert the flex item final positioning/sizing code to use logical coordinates & sizes.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 3a, v2: Make PositionTracker classes depend on whether axis is reversed, instead of whether axis is in physically positive direction

(Just tweaked some stale documentation about axes growing in a positive/negative direction.)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: part 3b: Convert flex item sizes & positions directly to logical coords

Here's part 3b, foretold in comment 12.

A few notes:
* Most of this patch is logicalizing (and documenting) some utility functions that convert out of flex-relative coordinates.
* This patch makes us use these functions to *directly* produce logical coords/sizes for our flex items' final reflow (instead of producing physical values & then manually converting those to logical versions, as current m-c does).
* This patch fixes a tangentially-related bug in contextual code (which is now important): The "containerWidth" variable was wrong up until this point, but it wasn't discoverable since we weren't exercising it with a non-default writing mode. It was the content-box width, when it really needs to be the border-box width (because the WM conversion code that uses it is with respect to the parent's frame-size, i.e. border-box).  Anyway, that's fixed here by adding the flex container's borderpadding LeftRight().

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8593498 [details] [diff] [review]
part 3b: Convert flex item sizes & positions directly to logical coords

(In reply to Daniel Holbert [:dholbert] from comment #14)
> It was the content-box width, when it really needs to be the
> border-box width

I made the same mistake :-)  (bug 1153140)

Jonathan has documented this now in the header file.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4988bb6de2f

Comment 17

[Daniel Holbert [:dholbert]]

Thanks for the reviews!

(In reply to Pulsebot from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d4988bb6de2f

(This push was parts 3a & 3b merged together. FWIW: before landing, I tweaked a XXXdholbert comment at end of part 3b to refer to "bug 1155312" instead of "next patch"; I spun off a separate bug for that patch, since I realized it didn't really have anything to do with the summary of this bug here.)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4988bb6de2f