Use logical dimensions/coordinates more in flex layout

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 2 bugs, {DevAdvocacy})

Trunk
mozilla60
DevAdvocacy
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr52 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

(Whiteboard: [DevRel:P2])

Attachments

(10 attachments)

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
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1223180
Keywords: DevAdvocacy
Whiteboard: [DevRel:P2]
(Assignee)

Updated

a year ago
Blocks: 1189131
(Assignee)

Comment 1

a year 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

a year ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

a year 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

a year 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

a year 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

a year 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 16

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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)

Updated

a year ago
Blocks: 1436875
(Assignee)

Comment 23

a year 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

a year 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

a year 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

a year 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)

Updated

a year ago
Blocks: 1436881
(Assignee)

Comment 27

a year 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

a year 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

a year 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
(Assignee)

Updated

a year ago
No longer blocks: 1223180
Duplicate of this bug: 1223180
(Assignee)

Updated

a year ago
Duplicate of this bug: 1189131
(Assignee)

Updated

a year ago
No longer blocks: 1189131
(Assignee)

Updated

a year ago
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox-esr52: --- → affected
Flags: in-testsuite+
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Updated

a year ago
Depends on: 1438704
status-firefox49: affected → wontfix
status-firefox58: affected → wontfix
status-firefox59: affected → wontfix
status-firefox-esr52: affected → wontfix
(Assignee)

Updated

a year ago
Blocks: 1430444
(Assignee)

Updated

a year ago
Blocks: 1385211
(Assignee)

Updated

a year ago
Blocks: 1276735
(Assignee)

Updated

a year ago
Duplicate of this bug: 1382867
Blocks: 1282940
(Assignee)

Updated

7 months ago
Depends on: 1486086
You need to log in before you can comment on or make changes to this bug.