Closed Bug 1267462 Opened 9 years ago Closed 7 years ago

Use logical dimensions/coordinates more in flex layout

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox-esr52 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: DevAdvocacy, Whiteboard: [DevRel:P2])

Attachments

(10 files)

Bug 1267462 part 1: Add copies of some existing flexbox reftests, now with orthogonal flows / vertical writing modes.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 2: Add a reftest for flex items with text & orthogonal flows.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 3: Add more reftests for flex items with text & orthogonal flows (as modified copies of previous patch's test).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
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).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 5: Move WritingMode member-var earlier in FlexItem class.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 6: Add FlexItem methods to test whether its inline axis is in container's main vs. cross axis.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 7: Generalize nsFlexContainerFrame::ResolveAutoFlexBasisAndMinSize and helpers to use logical axes & coords.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 8: Rename ReflowInput.mFlags.mIsFlexContainerMeasuringHeight with s/Height/BSize/, to match reality.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 9: Fix IsCrossSizeDefinite() helper-method to use logical axes & coords.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1267462 part 10: Fix flexbox code in nsFrame::ComputeSize[WithIntrinsicDimensions] to use logical axes & coords.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: 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.
Blocks: 1223180
Keywords: DevAdvocacy
Whiteboard: [DevRel:P2]
Blocks: 1189131
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: nobody → dholbert
Status: NEW → ASSIGNED
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 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 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 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 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 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 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 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 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 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+
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!
Blocks: 1436875
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.
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!
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.
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).
Blocks: 1436881
(I filed bug 1436881 for comment 21 / comment 25 - 26.)
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.
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
No longer blocks: 1223180
No longer blocks: 1189131
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox-esr52: --- → affected
Flags: in-testsuite+
OS: Unspecified → All
Hardware: Unspecified → All
Depends on: 1438704
Blocks: 1430444
Blocks: 1385211
Blocks: 1276735
Blocks: 1282940
Depends on: 1486086
Blocks: 1318825
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: