1173646 - Flex items are sized incorrectly if their writing-mode differs from the container

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase

STR:
 1. Load attached testcase.

EXPECTED RESULTS:
All three color-grids should look the same (and fully fill their black-outlined rect)

ACTUAL RESULTS:
The lower two grids (whose flex items' writing-modes differ from the flex container) have the wrong sizes; the items are skinnier.


The problem is here:
> 155 #define GET_MAIN_COMPONENT_LOGICAL(axisTracker_, isize_, bsize_)  \
> 156   (axisTracker_).IsRowOriented() ? (isize_) : (bsize_)
[...]
> 1061   nscoord flexBaseSize = GET_MAIN_COMPONENT_LOGICAL(aAxisTracker,
> 1062                                                     childRS.ComputedISize(),
> 1063                                                     childRS.ComputedBSize());

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=e0145b66ac03#1059

The ComputedISize()/BSize there are with respect to the *child*'s writing-mode -- but we're interpreting as if they were in the *container's* writing-mode.

We need to extend the GET_[MAIN|CROSS]_COMPONENT_LOGICAL macros to take a writing-mode corresponding to the passed-in isize/bsize.  If that writing mode is orthogonal to the container's writing-mode, we should return the opposite value that we would've returned.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: add reftest (marked as failing for now)

(I'm following dbaron's convention of adding the reftests first, marked as failing, and then marking them as passing in the patch that fixes things.)

Here's the first reftest. (similar to attached testcase; basically just repeats a flex container w/ 4 fixed-size children, with a variety of writing-mode values on the children.)

Next patch will add variants of this test with different writing-mode values on the flex container.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: add test variants w/ the other writing-mode values on the flex container

(This patch uses 'hg cp' to add 2 more reftests, to test this with the container having the other 2 possible 'writing-mode' values.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: (ignore; posted wrong patch)

This patch makes FlexItems cache their WritingMode, for quick access.

(We could use Frame()->GetWritingMode(), but that's a virtual function call & involves logic that we can skip, since we know the writing-mode up front when we construct the FlexItem.)

(For the "strut" FlexItem constructor (for flex items that have been collapsed via visibility:collapse and leave only a "strut" behind for sizing purposes), we just use the parent's WritingMode for simplicity & to avoid a really-unnecessary call to the child frame's virtual GetWritingMode() function.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 3: Make FlexItems cache their WritingMode

(Sorry, attached the wrong patch; here's the real "part 3")

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 4: Make GET_[MAIN|CROSS]_COMPONENT_LOGICAL take a writing-mode

Here's the main patch to actually fix this. This patch:
 - Extends GET_[MAIN|CROSS]_COMPONENT_LOGICAL so that callers pass a writing-mode along with an isize/bsize. (and then if the passed-in writing-mode is orthogonal to the container's writing-mode, we return the opposite input that we would've otherwise returned.)
 - Adds a convenience getter for the container's writing-mode to FlexboxAxisTracker. (I make use of this in other flexbox logicalization patches, too.)
 - Makes all calls to these getter-macros pass in the writing-mode that corresponds to their isize/bsize values, to be sure we interpret them correctly.
 - Removes the 'fails' annotations from reftest.list for the reftests added in parts 1 and 2.

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8620807 [details] [diff] [review]
part 4: Make GET_[MAIN|CROSS]_COMPONENT_LOGICAL take a writing-mode

r=mats

Comment 7

[Daniel Holbert [:dholbert]]

Thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/449d7f5271d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a9f3ac9f3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2accd3927c4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/660ab6f4e1be

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/449d7f5271d5
https://hg.mozilla.org/mozilla-central/rev/a3a9f3ac9f3b
https://hg.mozilla.org/mozilla-central/rev/2accd3927c4f
https://hg.mozilla.org/mozilla-central/rev/660ab6f4e1be