Closed Bug 1301014 Opened 4 years ago Closed 4 years ago
Intrinsic inline-size of flex container is wrong in vertical writing modes
58 bytes, text/x-review-board-request
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"
(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...)
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/613bb894d2c0 Fix intrinsic inline-size of flex container in vertical writing modes. r=dholbert
You need to log in before you can comment on or make changes to this bug.