1158290 - Use "ISize" instead of "Width", and "BSize" instead of "Height", throughout flex layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug to cover various s/Width/ISize/ and s/Height/BSize/ conversions throughout flex layout. (in support of bug 1079155)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Use AvailableBSize instead of AvailableHeight

This patch was auto-generated with the following command:
  sed -i "s/vailableHeight/vailableBSize/g" nsFlexContainerFrame.*

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: When subtracting border-padding start to determine availableBSizeForContent, use logical skipsides & borderpadding, instead of physical versions

Since the previous patch was automated, it leaves us in a somewhat inconsistent state, when working with the renamed values.

This patch converts the code that finalizes "availableBSizeForContent" (formerly "availableHeightForContent") to use logical information instead of physical information. (I converted/clarified the adjoining comment, too.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 3: Logicalize nsFlexContainerFrame::GenerateFlexLines (prevent it from assuming "aAvailableBSizeForContent" is a vertical quantity)

This patch converts nsFlexContainerFrame::GenerateFlexLines to be purely logical.

In particular:
 - Previously we checked if we were vertical (!IsMainAxisHorizontal) when deciding if we should try to wrap our flex items to avoid running out of AvailableHeight. Now, we need to check if we're "column-oriented" (flex-direction:column/column-reverse), i.e. "is our main axis in the block axis", when deciding if we should try to wrap our flex items to avoid running out of *AvailableBSize*.
 - I also logicalized a "max-main-size" lookup in adjacent code, which leaves GenerateFlexLines 100% logical.

Comment 4

[Mats Palmgren (inactive)]

Did you forget to ask for review on part 3, or is that part not done yet?

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8597471 [details] [diff] [review]
part 3: Logicalize nsFlexContainerFrame::GenerateFlexLines (prevent it from assuming "aAvailableBSizeForContent" is a vertical quantity)

r=mats assuming it's finished.

Comment 6

[Daniel Holbert [:dholbert]]

Yup, thanks -- meant to ask for review on part 3.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90e8896f13b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1b368f0857
https://hg.mozilla.org/integration/mozilla-inbound/rev/4722c39a16f7

Comment 8

[Daniel Holbert [:dholbert]]

(^ is parts 1, 2, and 3. More parts coming.)

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/90e8896f13b7
https://hg.mozilla.org/mozilla-central/rev/0c1b368f0857
https://hg.mozilla.org/mozilla-central/rev/4722c39a16f7

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 4: rename & better-document ClampFlexContainerMainSize()

This renames & improves the documentation of "ClampFlexContainerMainSize". This is the method that resolves our maybe-unresolved "tentative main size" into something that we can actually use for the Flex Layout Algorithm, using the distance to the next page break, and the lengths of our flex lines.

This patch is basically all (minor) renaming and documentation. It's not ISize/BSize-related; I'm only putting it on this bug because it sets the stage more clearly for the next patch (which will do ISize/BSize cleanup in this function).

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: part 5: logicalize functions about computing main/cross size of flex container

This patch removes width/height/vertical/horizontal language (and does related cleanup/clarification) in 3 functions, which are related to computing the main & cross size of our flex container:
 1) nsFlexContainerFrame::GetMainSizeFromReflowState, which just grabs a "tentative" size from the reflow state (which we use to form flex lines).

 2) ResolveFlexContainerMainSize(), which further-resolves that "tentative" size once we know how big our lines are. (Formerly ClampFlexContainerMainSize; renamed in previous patch)

 3) nsFlexContainerFrame::ComputeCrossSize(), which is the same as those ^ two functions combined, but for the cross axis.

As noted in several XXXdholbert comments in the patch, right now it's possible for us to have an unconstrained ComputedISize; I *think* this is a bug in our nsHTMLReflowState code, and that's tracked over in bug 1163238.  (For now, that just means situations that trigger this -- a flex container whose writing-mode differs from its praent, basically -- can end up being NS_UNCONSTRAINEDSIZE tall. This is silly, but can be fixed separately from this bug; probably in bug 1163238 or a followup.)

As a side effect, this patch fixes bug 1149383 (some test failures in flexbox + writing-mode reftests), so I'm updating the reftest manifest here to reflect that.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: part 6: Switch GET_MAIN_COMPONENT / GET_CROSS_COMPONENT calls to use logical versions

This patch reduces explicit width/height usage by using some _LOGICAL macros (which take args "ISize, BSize" & select the main/cross-axis arg) in place of physical macros (which take args "width, height" & select the main/cross-axis one).

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8606132 [details] [diff] [review]
part 5: logicalize functions about computing main/cross size of flex container

The added comment on line 3318-3319 feels a bit redundant.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43404a28955d
https://hg.mozilla.org/integration/mozilla-inbound/rev/514f50da0d6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/49cdc55a2121

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> Comment on attachment 8606132 [details] [diff] [review]
> part 5: logicalize functions about computing main/cross size of flex
> container
> 
> The added comment on line 3318-3319 feels a bit redundant.

Fair enough -- I removed both versions of that comment before landing (the one in ResolveFlexContainerMainSize & the one in ComputeCrossSize). Thanks for the reviews!

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/43404a28955d
https://hg.mozilla.org/mozilla-central/rev/514f50da0d6d
https://hg.mozilla.org/mozilla-central/rev/49cdc55a2121

Comment 17

[Daniel Holbert [:dholbert]]

(I have another couple patches that I'd meant to post here -- but, given that the trains have now changed tracks & having a single bug span work that happens in multiple releases is awkward, I'm going to close this out and file a new bug for remaining patches.)