Closed
Bug 1267462
Opened 5 years ago
Closed 3 years ago
Use logical dimensions/coordinates more in flex layout
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 2 open bugs)
Details
(Keywords: DevAdvocacy, Whiteboard: [DevRel:P2])
Attachments
(10 files)
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
mats
:
review+
|
Details |
Spinning this bug off of bug 1180107 (which I'm morphing). This bug is a placeholder for the remaining work that needs to be done to convert from physical to logical coordinates & axes in flex layout.
|
||
Updated•4 years ago
|
Keywords: DevAdvocacy
Whiteboard: [DevRel:P2]
|
Assignee | |
Comment 1•3 years ago
|
||
I've got a patch stack ready for this bug, split into several test patches & several code patches. I'll push the test patches to mozreview first, and the code patches will follow later on today (appending to the stack in mozreview).
|
|
Assignee | |
Updated•3 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 6•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949453 [details] Bug 1267462 part 2: Add a reftest for flex items with text & orthogonal flows. https://reviewboard.mozilla.org/r/218752/#review224586 ::: layout/reftests/w3c-css/submitted/flexbox/flexbox-writing-mode-010-ref.html:11 (Diff revision 1) > +<html> > +<head> > + <title>CSS Reftest Reference</title> > + <meta charset="utf-8"> > + <link rel="author" title="Daniel Holbert" href="mailto:dholbert@mozilla.com"> > + <link rel="stylesheet" type="text/css" href="support/ahem.css"> nit: I seem to vaguely recall that including the Ahem font using @font-face is frowned upon in WPT, so we should probably address this in general at some point.
Attachment #8949453 -
Flags: review?(mats) → review+
|
|
||
Comment 7•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949454 [details] Bug 1267462 part 3: Add more reftests for flex items with text & orthogonal flows (as modified copies of previous patch's test). https://reviewboard.mozilla.org/r/218754/#review224588
Attachment #8949454 -
Flags: review?(mats) → review+
|
|
||
Comment 8•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949455 [details] Bug 1267462 part 4: Add more reftests for flex items with text & orthogonal flows (as modified copies of previous patch's final column-oriented test). https://reviewboard.mozilla.org/r/218756/#review224590
Attachment #8949455 -
Flags: review?(mats) → review+
|
|
||
Comment 9•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949452 [details] Bug 1267462 part 1: Add copies of some existing flexbox reftests, now with orthogonal flows / vertical writing modes. https://reviewboard.mozilla.org/r/218750/#review224580
Attachment #8949452 -
Flags: review?(mats) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 16•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949525 [details] Bug 1267462 part 5: Move WritingMode member-var earlier in FlexItem class. https://reviewboard.mozilla.org/r/218862/#review224634 ::: layout/generic/nsFlexContainerFrame.cpp:809 (Diff revision 1) > - nsIFrame* const mFrame; > - > + nsIFrame* const mFrame; // The flex item's frame. > + const WritingMode mWM; // The flex item's writing mode. > - // Values that we already know in constructor: (and are hence mostly 'const') > const float mFlexGrow; AFAICT, this creates a 3-byte alignment padding though. It looks like we can avoid that by adding it before mIsFrozen instead. r=mats with that
Attachment #8949525 -
Flags: review?(mats) → review+
|
|
||
Comment 17•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949526 [details] Bug 1267462 part 6: Add FlexItem methods to test whether its inline axis is in container's main vs. cross axis. https://reviewboard.mozilla.org/r/218864/#review224638
Attachment #8949526 -
Flags: review?(mats) → review+
|
|
||
Comment 18•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949527 [details] Bug 1267462 part 7: Generalize nsFlexContainerFrame::ResolveAutoFlexBasisAndMinSize and helpers to use logical axes & coords. https://reviewboard.mozilla.org/r/218866/#review224640 ::: layout/generic/nsFlexContainerFrame.cpp:1630 (Diff revision 1) > - // 'min-height:auto'? > + // 'min-{bsize}:auto'? > Perhaps min-block-size:auto is clearer since it's an actual property name? ::: layout/generic/nsFlexContainerFrame.cpp:2040 (Diff revision 1) > - // If the cross axis is horizontal, then changes to the item's main size > - // (height) can't influence its cross size (width), if the item is a block > - // with a horizontal writing-mode. > + // If we get here, this function is really asking: "can changes to this > + // item's block size have an influence on its inline size"? For blocks and > + // tables, the answer is "no". FWIW, for GridContainerFrames, I think the bsize can only affect the isize if it has "grid-auto-flow:column" and there is a repeat(auto-fill/fit) in the block-axis, which is rare.
Attachment #8949527 -
Flags: review?(mats) → review+
|
|
||
Comment 19•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949528 [details] Bug 1267462 part 8: Rename ReflowInput.mFlags.mIsFlexContainerMeasuringHeight with s/Height/BSize/, to match reality. https://reviewboard.mozilla.org/r/218868/#review224644
Attachment #8949528 -
Flags: review?(mats) → review+
|
|
||
Comment 20•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949529 [details] Bug 1267462 part 9: Fix IsCrossSizeDefinite() helper-method to use logical axes & coords. https://reviewboard.mozilla.org/r/218870/#review224652 ::: layout/generic/nsFlexContainerFrame.cpp:1346 (Diff revision 1) > +// The logic here should be similar to the logic for isAutoISize/IsAutoBSize > // in nsFrame::ComputeSizeWithIntrinsicDimensions(). > static bool nit: looking at nsFrame::ComputeSizeWithIntrinsicDimensions, I suspect that they should both start with "i".
Attachment #8949529 -
Flags: review?(mats) → review+
|
|
||
Comment 21•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949530 [details] Bug 1267462 part 10: Fix flexbox code in nsFrame::ComputeSize[WithIntrinsicDimensions] to use logical axes & coords. https://reviewboard.mozilla.org/r/218872/#review224656 ::: layout/generic/nsFrame.cpp:5669 (Diff revision 1) > - if (isInlineFlexItem) { > - inlineStyleCoord = flexBasis; > - } else { > - // One caveat for vertical flex items: We don't support enumerated > - // values (e.g. "max-content") for height properties yet. So, if our > - // computed flex-basis is an enumerated value, we'll just behave as if > - // it were "auto", which means "use the main-size property after all" > - // (which is "height", in this case). > - // NOTE: Once we support intrinsic sizing keywords for "height", > - // we should remove this check. > - if (flexBasis->GetUnit() != eStyleUnit_Enumerated) { > - blockStyleCoord = flexBasis; > - } > + // One caveat for when flex-basis is stomping on 'height': We don't > + // support enumerated values (e.g. "max-content") for height yet (that's > + // bug 567039). So, if our computed flex-basis is an enumerated value, > + // we'll just behave as if it were "auto", which means "use the main-size > + // property after all" (which is "height", in this case). NOTE: Once we > + // support intrinsic sizing keywords for "height", we should remove this > + // check. > + bool usingFlexBasisForHeight = > + (usingFlexBasisForISize != aWM.IsVertical()); > + if (!usingFlexBasisForHeight || > + flexBasis->GetUnit() != eStyleUnit_Enumerated) { > + // Override whichever coord we're overriding: > + (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) = > + flexBasis; > } It seems odd to me that this chunk is still talking about physical height, shouldn't it be s/height/bsize/ everywhere here?
Attachment #8949530 -
Flags: review?(mats) → review+
|
|
Assignee | |
Comment 22•3 years ago
|
||
| mozreview-review | ||
Comment on attachment 8949525 [details] Bug 1267462 part 5: Move WritingMode member-var earlier in FlexItem class. https://reviewboard.mozilla.org/r/218862/#review224658 ::: layout/generic/nsFlexContainerFrame.cpp:809 (Diff revision 1) > - nsIFrame* const mFrame; > - > + nsIFrame* const mFrame; // The flex item's frame. > + const WritingMode mWM; // The flex item's writing mode. > - // Values that we already know in constructor: (and are hence mostly 'const') > const float mFlexGrow; Works for me. Fixed locally -- thanks!
|
|
Assignee | |
Comment 23•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8949527 [details] Bug 1267462 part 7: Generalize nsFlexContainerFrame::ResolveAutoFlexBasisAndMinSize and helpers to use logical axes & coords. https://reviewboard.mozilla.org/r/218866/#review224640 > Perhaps min-block-size:auto is clearer since it's an actual property name? Sure -- I've made this change. > FWIW, for GridContainerFrames, I think the bsize can only affect the isize if it has "grid-auto-flow:column" and there is a repeat(auto-fill/fit) in the block-axis, which is rare. Good to know. I filed 1436875 as a followup, for further investigations on this.
|
|
Assignee | |
Comment 24•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8949529 [details] Bug 1267462 part 9: Fix IsCrossSizeDefinite() helper-method to use logical axes & coords. https://reviewboard.mozilla.org/r/218870/#review224652 > nit: looking at nsFrame::ComputeSizeWithIntrinsicDimensions, I suspect that they should both start with "i". Good catch. Fixed locally - thanks!
|
|
Assignee | |
Comment 25•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8949530 [details] Bug 1267462 part 10: Fix flexbox code in nsFrame::ComputeSize[WithIntrinsicDimensions] to use logical axes & coords. https://reviewboard.mozilla.org/r/218872/#review224656 > It seems odd to me that this chunk is still talking about physical height, shouldn't it be s/height/bsize/ everywhere here? On re-review, you're right -- I think it should. I'd initially gone in that direction, but then I found bug 567039, and I (mistakenly) thought that bug meant we still didn't support the enumerated keywords, specifically for 'height'. But I just tested them & inspected property_database.js, and it looks like we sort-of do support them now (in vertical writing-modes at least, and treat them as 'auto' otherwise), as of bug 1122253. So: I'll adjust this to be about the block axis instead of the height/vertical axis, as you suggested.
|
|
Assignee | |
Comment 26•3 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8949530 [details] Bug 1267462 part 10: Fix flexbox code in nsFrame::ComputeSize[WithIntrinsicDimensions] to use logical axes & coords. https://reviewboard.mozilla.org/r/218872/#review224656 > On re-review, you're right -- I think it should. > > I'd initially gone in that direction, but then I found bug 567039, and I (mistakenly) thought that bug meant we still didn't support the enumerated keywords, specifically for 'height'. But I just tested them & inspected property_database.js, and it looks like we sort-of do support them now (in vertical writing-modes at least, and treat them as 'auto' otherwise), as of bug 1122253. > > So: I'll adjust this to be about the block axis instead of the height/vertical axis, as you suggested. Actually, I think I'm going to spin this off into a followup bug -- it's not really an integral part of this bug. And I think we can come up with a better behavior (& better test-coverage) for "-moz-min-content" & friends in flex-basis, anyway, and I don't want to hold this bug up on that. So for now, this piece will just be preserving the intent of the original code here, RE "-moz-min-content" & friends for "flex-basis" (honoring it if the flex-basis axis is literally horizontal, & treating it as 'auto' otherwise).
|
|
Assignee | |
Comment 27•3 years ago
|
||
(I filed bug 1436881 for comment 21 / comment 25 - 26.)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 38•3 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f54264cc7b4531d536333e96709c38dc5ed982a0 I believe all the failures there are unrelated intermittents. I'll land on autoland shortly.
|
||
Comment 39•3 years ago
|
||
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b370336ae126 part 1: Add copies of some existing flexbox reftests, now with orthogonal flows / vertical writing modes. r=mats https://hg.mozilla.org/integration/autoland/rev/19aed0c8a848 part 2: Add a reftest for flex items with text & orthogonal flows. r=mats https://hg.mozilla.org/integration/autoland/rev/05619c4f4a70 part 3: Add more reftests for flex items with text & orthogonal flows (as modified copies of previous patch's test). r=mats https://hg.mozilla.org/integration/autoland/rev/2a4fea53334e part 4: Add more reftests for flex items with text & orthogonal flows (as modified copies of previous patch's final column-oriented test). r=mats https://hg.mozilla.org/integration/autoland/rev/60666668cf81 part 5: Move WritingMode member-var earlier in FlexItem class. r=mats https://hg.mozilla.org/integration/autoland/rev/ceef95f707fe part 6: Add FlexItem methods to test whether its inline axis is in container's main vs. cross axis. r=mats https://hg.mozilla.org/integration/autoland/rev/1c67959201d2 part 7: Generalize nsFlexContainerFrame::ResolveAutoFlexBasisAndMinSize and helpers to use logical axes & coords. r=mats https://hg.mozilla.org/integration/autoland/rev/153279d8dcfe part 8: Rename ReflowInput.mFlags.mIsFlexContainerMeasuringHeight with s/Height/BSize/, to match reality. r=mats https://hg.mozilla.org/integration/autoland/rev/d28772595a77 part 9: Fix IsCrossSizeDefinite() helper-method to use logical axes & coords. r=mats https://hg.mozilla.org/integration/autoland/rev/3a096d205f99 part 10: Fix flexbox code in nsFrame::ComputeSize[WithIntrinsicDimensions] to use logical axes & coords. r=mats
|
||
Comment 40•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/b370336ae126 https://hg.mozilla.org/mozilla-central/rev/19aed0c8a848 https://hg.mozilla.org/mozilla-central/rev/05619c4f4a70 https://hg.mozilla.org/mozilla-central/rev/2a4fea53334e https://hg.mozilla.org/mozilla-central/rev/60666668cf81 https://hg.mozilla.org/mozilla-central/rev/ceef95f707fe https://hg.mozilla.org/mozilla-central/rev/1c67959201d2 https://hg.mozilla.org/mozilla-central/rev/153279d8dcfe https://hg.mozilla.org/mozilla-central/rev/d28772595a77 https://hg.mozilla.org/mozilla-central/rev/3a096d205f99
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
|
|
Assignee | |
Updated•3 years ago
|
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → affected
Flags: in-testsuite+
OS: Unspecified → All
Hardware: Unspecified → All
|
||
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•