Closed Bug 1173646 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Attached file 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.
(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.
Attachment #8620798 - Flags: review?(mats)
(This patch uses 'hg cp' to add 2 more reftests, to test this with the container having the other 2 possible 'writing-mode' values.)
Blocks: 1123299
Attached patch (ignore; posted wrong patch) (obsolete) — Splinter Review
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.)
Attachment #8620804 - Flags: review?(mats)
(Sorry, attached the wrong patch; here's the real "part 3")
Attachment #8620804 - Attachment is obsolete: true
Attachment #8620804 - Flags: review?(mats)
Attachment #8620805 - Flags: review?(mats)
Attachment #8620804 - Attachment description: part 3: Make FlexItems cache their WritingMode → (ignore; posted wrong patch)
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.
Attachment #8620807 - Flags: review?(mats)
Attachment #8620798 - Flags: review?(mats) → review+
Attachment #8620805 - Flags: review?(mats) → review+
Comment on attachment 8620807 [details] [diff] [review]
part 4: Make GET_[MAIN|CROSS]_COMPONENT_LOGICAL take a writing-mode

r=mats
Attachment #8620807 - Flags: review?(mats) → review+
Thanks!
You need to log in before you can comment on or make changes to this bug.