Closed Bug 1301014 Opened 3 years ago Closed 3 years ago

Intrinsic inline-size of flex container is wrong in vertical writing modes

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

Details

Attachments

(1 file)

Summary: Intrinsic inline-size of flexbox container is wrong in vertical writing modes → Intrinsic inline-size of flex container is wrong in vertical writing modes
Comment on attachment 8788811 [details]
Bug 1301014 - Fix intrinsic inline-size of flex container in vertical writing modes.

https://reviewboard.mozilla.org/r/77166/#review75502

Thanks! Just to be clear - the IsRowOriented() changes are the only functional changes here, I think, right? (Everything else is just renaming?)

You might consider splitting the variable-renamings (90% of this patch) out into their own patch, to make the functional change(s) clearer -- but this is small enough that maybe it's not worth it.

Also: could you include an automated test?  (Holding off on r+ for the moment, for that reason, but otherwise this looks good.)

::: layout/generic/nsFlexContainerFrame.cpp:4411
(Diff revision 1)
>                                             nsLayoutUtils::MIN_ISIZE);
>      // For a horizontal single-line flex container, the intrinsic min width is
>      // the sum of its items' min widths.
>      // For a vertical flex container, or for a multi-line horizontal flex
>      // container, the intrinsic min width is the max of its items' min widths.
> -    if (axisTracker.IsMainAxisHorizontal() &&
> +    if (axisTracker.IsRowOriented() &&

Please fix the comment above this line to keep up with the code:
 s/horizontal/row-oriented/ (two instances)
 s/vertical/column-oriented/
 s/width/isize/
Also, the commit message is currently:
> Bug 1301014 - Fix intrinsic inline-size of flex container. r?dholbert

That should probably describe the change (or what's being fixed) more specifically. e.g:
  * Maybe just add "in vertical writing modes" at the end
  * Or add: "to depend on row/column orientation instead of horizontalness"
Flags: needinfo?(xidorn+moz)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Also: could you include an automated test?  (Holding off on r+ for the
> moment, for that reason, but otherwise this looks good.)

I'm not very willing to add a test for this, because there is an existing test in CSSWG test repo for Writing Modes, and I want to import all of those tests in bug 1258916...

(I think I've avoided adding test because of the same reason in some other bug, so I probably really should try harder to get bug 1258916 landed...)
Flags: needinfo?(xidorn+moz)
OK, that sounds fine then.

Assuming you've verified that this patch makes the test pass, and the test is landing soon, then I'm happy to call this r=me with the other nits in comment 3 and 4 addressed.
Attachment #8788811 - Flags: review?(dholbert) → review+
Attachment #8788811 - Flags: review?(dholbert) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/613bb894d2c0
Fix intrinsic inline-size of flex container in vertical writing modes. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/613bb894d2c0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.