Closed Bug 1147834 Opened 6 years ago Closed 5 years ago

Convert remaining uses of physical coordinates in nsHTMLReflowState

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: smontagu, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 16 obsolete files)

11.40 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
143.85 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
Following bug 1130907 comment 3, we need to convert the remaining places in nsHTMLReflowState that don't use abstract coordinates.
Initialize CSSOffsetState consistently with the containing block inline size instead of width, and rename nsTableOuterFrame::ChildShrinkWrapWidth to ChildShrinkWrapISize (which in practice is already what it does)
Assignee: nobody → smontagu
This is a rename-only patch to make other changes easier to follow.
These tests fail before patch 3 and pass after it.
Parts of this were obsoleted by bug 1156021
Attachment #8584308 - Attachment is obsolete: true
Attachment #8599173 - Attachment description: Patch 2: rename ISize/BSize to width/height throughout nsHTMLReflowState → Patch 2: rename width/height to ISize/BSize throughout nsHTMLReflowState
Attachment 8599173 [details] [diff] causes reftest regressions in:

layout/reftests/floats/float-in-rtl-vlr-1[abcd].html
layout/reftests/floats/float-in-rtl-vrl-1[abcd].html
layout/reftests/writing-mode/1108923-1-percentage-margins.html
This causes an additional regression in layout/reftests/writing-mode/1102175-1b.html
This fixes the regressions listed in comment 6, and also causes (possibly bogus) UNEXPECTED-PASS in 
layout/reftests/floats/float-in-rtl-vrl-3[abcd].html

It causes a new regression in
layout/reftests/writing-mode/1158549-1-vertical-block-size-constraints.html
This fixes the regression in layout/reftests/writing-mode/1158549-1-vertical-block-size-constraints.html
No longer blocks: 1130907
Comment on attachment 8599173 [details] [diff] [review]
Patch 2: rename width/height to ISize/BSize throughout nsHTMLReflowState

Review of attachment 8599173 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.h
@@ +629,5 @@
>                      const nsHTMLReflowState&    aParentReflowState,
>                      nsIFrame*                   aFrame,
>                      const mozilla::LogicalSize& aAvailableSpace,
> +                    nscoord                     aContainingBlockISize = -1,
> +                    nscoord                     aContainingBlockBSize = -1,

I think I'd like us to change this to pass aContainingBlockSize as a LogicalSize, so that (a) it'll force us to look at all call-sites to check what they're passing, and (b) it'll allow debug builds to check the writing mode. (And similarly for the various other methods that take containing block dimensions.)
Comment on attachment 8599215 [details] [diff] [review]
Patch 4: use abstract coordinates when calculating margins, borders and paddingpatch 4: use abstract coordinates when calculating margins, borders and padding

Review of attachment 8599215 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +1177,5 @@
> +  //'inline size', see if we can get the intrinsic size. This will allow
> +  // us to exactly determine both the inline edges
> +  WritingMode wm = GetWritingMode();
> +  nsStyleCoord styleISize = wm.IsVertical()
> +    ? mStylePosition->mWidth : mStylePosition->mHeight;

Note that bug 1159305 proposes to offer mStylePosition->ISize(wm) as a convenient shorthand for this.
Comment on attachment 8599226 [details] [diff] [review]
Patch 5: use logical border and padding when calculating containing block sizes

Review of attachment 8599226 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +2069,5 @@
>                                             cbSize.BSize(cbwm)),
>                  aFrameType, aBorder, aPadding);
>  
> +    const nsStyleCoord &blockSize = cbwm.IsVertical()
> +      ? mStylePosition->mWidth : mStylePosition->mHeight;

Again, see bug 1159305 (and similarly for inlineSize, below).
Comment on attachment 8599215 [details] [diff] [review]
Patch 4: use abstract coordinates when calculating margins, borders and paddingpatch 4: use abstract coordinates when calculating margins, borders and padding

Review of attachment 8599215 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +1175,5 @@
> +
> +  // If it's a replaced element and it has a 'auto' value for
> +  //'inline size', see if we can get the intrinsic size. This will allow
> +  // us to exactly determine both the inline edges
> +  WritingMode wm = GetWritingMode();

I suspect (but am open to debate!) that we want to be using cbrs->GetWritingMode() in this method.
Comment on attachment 8599215 [details] [diff] [review]
Patch 4: use abstract coordinates when calculating margins, borders and paddingpatch 4: use abstract coordinates when calculating margins, borders and padding

Review of attachment 8599215 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +1908,5 @@
>      // an element in quirks mode gets a containing block based on looking for a
>      // parent with a non-auto height if the element has a percent height
> +    // Note: We don't emulate this quirk for percents in calc() or in
> +    // vertical writing modes.
> +    if (!GetWritingMode().IsVertical() &&

I believe this should be checking aContainingBlockRS->GetWritingMode().IsVertical(), not our own writing-mode.

Fixing this resolves at least some of the reftest failures this patch series currently causes.
Comment on attachment 8599215 [details] [diff] [review]
Patch 4: use abstract coordinates when calculating margins, borders and paddingpatch 4: use abstract coordinates when calculating margins, borders and padding

Review of attachment 8599215 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +1177,5 @@
> +  //'inline size', see if we can get the intrinsic size. This will allow
> +  // us to exactly determine both the inline edges
> +  WritingMode wm = GetWritingMode();
> +  nsStyleCoord styleISize = wm.IsVertical()
> +    ? mStylePosition->mWidth : mStylePosition->mHeight;

Oh, and it looks like mWidth and mHeight are reversed here! That probably explains some failures. Switching to the ISize(wm) accessor will make that slip less likely.
In the course of looking at position:absolute, I've been using this modification to the code from patch 4 (after patch 8 in the queue here, so as not to conflict with the later patches).
Attachment #8608575 - Flags: feedback?(smontagu)
Assignee: smontagu → jfkthame
Status: NEW → ASSIGNED
Attachment #8599215 - Attachment is obsolete: true
Attachment #8608760 - Attachment description: Patch 4: use abstract coordinates when calculating margins, borders and paddingpatch 4 updated to comments: use abstract coordinates when calculating margins, borders and padding → Patch 4 updated to comments: use abstract coordinates when calculating margins, borders and padding
Comment on attachment 8608766 [details] [diff] [review]
Patch 10: Use a single LogicalSize argument in nsHTMLReflowState instead of aContainingBlockWidth and aContainingBlockHeight

Sorry! some of this is going to conflict with your work in bug 1079151
Attachment #8608766 - Flags: feedback?(jfkthame)
The try push has many occurences of this assertion: 

ASSERTION: containing block height must be constrained: 'aCBSize.BSize(wm) != nscoord(1 << 30)', file /builds/slave/try-lx-d-000000000000000000000/build/src/layout/generic/nsHTMLReflowState.cpp, line 1436
See https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a5f64e461aa for a try push built on top of these patches, including a fix for most of those assertions. There are just a few remaining that need to be checked....

(I don't think it works to use IsAllZero() as the test for "explicit containing block was not provided", as we may pass one that's explicitly zero. See https://hg.mozilla.org/try/rev/f47609a6f02c.)
(Sorry, didn't mean to re-assign this bug; bzexport fail.)

I'm updating my patches for bug 1079151 to build on top of patch 10 here, which I think is the right way to go. I've got a few adjustments to it that I'm using locally and that appear to resolve the various assertions seen in comment 25. Will attach a followup here.
Assignee: jfkthame → smontagu
Attached patch Followup to patch 10 (obsolete) — Splinter Review
Proposed fixes to patch 10.
Attachment #8609999 - Flags: feedback?(smontagu)
I believe the patch above now fixes all the assertions here; see https://treeherder.mozilla.org/#/jobs?repo=try&revision=5add3e855e44.

The remaining reftest failures are related to position:absolute (in either the testcases or their references), and are fixed by the latest patches in bug 1079151; see https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6a361fd18c0.
Comment on attachment 8609999 [details] [diff] [review]
Followup to patch 10

Review of attachment 8609999 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +380,5 @@
>  
>    InitFrameType(type);
>    InitCBReflowState();
>  
> +  LogicalSize cbSize(mWritingMode, -1, -1);

I take your point that using an empty LogicalSize here is ambiguous, but in any case I don't like what I did here: in effect we're checking in two different places (here and inside InitConstraints) whether the caller passed in containing block dimensions. I would much rather remove this altogether if possible.

@@ +2058,2 @@
>  
>      const nsStyleCoord &blockSize = mStylePosition->BSize(cbwm);

Is it correct that this still uses the containing block writing mode even though you changed the calculations below to use the "native" writing mode?
Attachment #8583784 - Flags: review?(jfkthame)
Attachment #8599173 - Flags: review?(jfkthame)
Attachment #8584309 - Flags: review?(jfkthame)
Attachment #8584310 - Flags: review?(jfkthame)
Attachment #8599231 - Flags: review?(jfkthame)
Attachment #8599233 - Flags: review?(jfkthame)
Attachment #8599690 - Flags: review?(jfkthame)
Comment on attachment 8608575 [details] [diff] [review]
patch 9: followup to patch 4, use containing block's writing mode in CalculateHypotheticalBox

This is incorporated in the later version of patch 4
Attachment #8608575 - Attachment is obsolete: true
Attachment #8608575 - Flags: feedback?(smontagu)
Attachment #8608760 - Flags: review?(jfkthame)
Attachment #8608763 - Flags: review?(jfkthame)
(In reply to Simon Montagu :smontagu from comment #31)
> Comment on attachment 8609999 [details] [diff] [review]
> Followup to patch 10
> 
> Review of attachment 8609999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsHTMLReflowState.cpp
> @@ +380,5 @@
> >  
> >    InitFrameType(type);
> >    InitCBReflowState();
> >  
> > +  LogicalSize cbSize(mWritingMode, -1, -1);
> 
> I take your point that using an empty LogicalSize here is ambiguous, but in
> any case I don't like what I did here: in effect we're checking in two
> different places (here and inside InitConstraints) whether the caller passed
> in containing block dimensions. I would much rather remove this altogether
> if possible.
> 
> @@ +2058,2 @@
> >  
> >      const nsStyleCoord &blockSize = mStylePosition->BSize(cbwm);
> 
> Is it correct that this still uses the containing block writing mode even
> though you changed the calculations below to use the "native" writing mode?

No, that looks wrong; it should be |wm| to match what's below, I think.

I assume you're going to update patch 10, and will deal with this there? So I won't post a fixed version of this for now.
(In reply to Jonathan Kew (:jfkthame) from comment #33)
> I assume you're going to update patch 10, and will deal with this there? So
> I won't post a fixed version of this for now.

You assumption is correct.
Comment on attachment 8584310 [details] [diff] [review]
Tests for overconstrained relative positioning

Review of attachment 8584310 [details] [diff] [review]:
-----------------------------------------------------------------

This patch, as well as the preceding ones, refers to bug 1130907 in its commit message, and in the testcase filenames here. Those should all be updated to 1147834, I guess.

::: layout/reftests/writing-mode/reftest.list
@@ +110,5 @@
> +== 1130907-relative-overconstrained-horizontal-tb-rtl.html 1130907-bottom-left-ref.html
> +== 1130907-relative-overconstrained-vertical-lr-ltr.html 1130907-bottom-right-ref.html
> +== 1130907-relative-overconstrained-vertical-lr-rtl.html 1130907-top-right-ref.html
> +== 1130907-relative-overconstrained-vertical-rl-ltr.html 1130907-bottom-left-ref.html
> +== 1130907-relative-overconstrained-vertical-rl-rtl.html 1130907-top-left-ref.html

Unfortunately, the patch in bug 1167930 (Handle direction:rtl in vertical modes when converting a LogicalMargin to physical) appears to break the two vertical-*-rtl testcases here.

I still think the bug 1167930 patch is correct, though, so this needs further investigation; I haven't pinned down exactly why it's causing this failure yet.
Comment on attachment 8583784 [details] [diff] [review]
Patch 1: initialize CSSOffsetState with the containing block inline size

Review of attachment 8583784 [details] [diff] [review]:
-----------------------------------------------------------------

Please remember to fix the bug number in the commit message!

::: layout/tables/nsTableOuterFrame.cpp
@@ +361,5 @@
>    return maxWidth;
>  }
>  
>  // Compute the margin-box width of aChildFrame given the inputs.  If
>  // aMarginResult is non-null, fill it with the part of the margin-width

s/width/isize/ in the comment, for consistency with updated code
Attachment #8583784 - Flags: review?(jfkthame) → review+
Comment on attachment 8599173 [details] [diff] [review]
Patch 2: rename width/height to ISize/BSize throughout nsHTMLReflowState

Review of attachment 8599173 [details] [diff] [review]:
-----------------------------------------------------------------

Bug number in commit message.

Actually, I wonder if a number of the patches here should be folded together before landing, as they really don't make sense individually; they're logical steps towards a coherent result, but the intermediate stages can't reasonably stand by themselves.
Attachment #8599173 - Flags: review?(jfkthame) → review+
Comment on attachment 8584309 [details] [diff] [review]
Patch 3: use abstract coordinates in ComputeRelativeOffsets

Review of attachment 8584309 [details] [diff] [review]:
-----------------------------------------------------------------

s/1130907/1147834/ in commit message

::: layout/generic/nsHTMLReflowState.cpp
@@ +861,1 @@
>      // 'Left' isn't 'auto' so compute its value

s/Left/inline-start/

@@ +878,3 @@
>  
>    // Check for percentage based values and a containing block height that
>    // depends on the content height. Treat them like 'auto'

s/height/block-size/ (twice)

@@ +887,4 @@
>      }
>    }
>  
>    // If neither is 'auto', 'bottom' is ignored

s/bottom/block-end/
Attachment #8584309 - Flags: review?(jfkthame) → review+
Comment on attachment 8584310 [details] [diff] [review]
Tests for overconstrained relative positioning

Review of attachment 8584310 [details] [diff] [review]:
-----------------------------------------------------------------

The testcases look correct, AFAICS; the two expected failures are due to bug 1131451, as discussed in 1167930.

::: layout/reftests/writing-mode/reftest.list
@@ +108,5 @@
>  HTTP(..) == 1127488-align-right-vertical-lr-ltr.html 1127488-align-bottom-left-ref.html
> +== 1130907-relative-overconstrained-horizontal-tb-ltr.html 1130907-bottom-right-ref.html
> +== 1130907-relative-overconstrained-horizontal-tb-rtl.html 1130907-bottom-left-ref.html
> +== 1130907-relative-overconstrained-vertical-lr-ltr.html 1130907-bottom-right-ref.html
> +== 1130907-relative-overconstrained-vertical-lr-rtl.html 1130907-top-right-ref.html

annotate as 'fails', assuming bug 1147834 is going to land before this.

@@ +110,5 @@
> +== 1130907-relative-overconstrained-horizontal-tb-rtl.html 1130907-bottom-left-ref.html
> +== 1130907-relative-overconstrained-vertical-lr-ltr.html 1130907-bottom-right-ref.html
> +== 1130907-relative-overconstrained-vertical-lr-rtl.html 1130907-top-right-ref.html
> +== 1130907-relative-overconstrained-vertical-rl-ltr.html 1130907-bottom-left-ref.html
> +== 1130907-relative-overconstrained-vertical-rl-rtl.html 1130907-top-left-ref.html

ditto
Attachment #8584310 - Flags: review?(jfkthame) → review+
> annotate as 'fails', assuming bug 1147834 is going to land before this.

I mean 1167930, of course.
Attachment #8608760 - Flags: review?(jfkthame) → review+
Attachment #8608763 - Flags: review?(jfkthame) → review+
Comment on attachment 8599231 [details] [diff] [review]
Patch 6: use inline and block size as the basis of percentage margin and padding sizes

Review of attachment 8599231 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +2667,5 @@
> +  WritingMode wm = GetWritingMode();
> +  mozilla::css::Side inlineStart = wm.PhysicalSide(eLogicalSideIStart);
> +  mozilla::css::Side inlineEnd   = wm.PhysicalSide(eLogicalSideIEnd);
> +  mozilla::css::Side blockStart  = wm.PhysicalSide(eLogicalSideBStart);
> +  mozilla::css::Side blockEnd    = wm.PhysicalSide(eLogicalSideBEnd);

Rather than setting up these variables (which are only used once each), it'd be more concise to use the logical accessors like

  styleMargin->mMargin.GetIStart(wm)

etc below.

(In cases where they're used repeatedly, it may be fractionally more efficient to do the logical-to-physical mapping only once for each logical side and record it in a local var, but in this method there's no benefit to that.)

Hmm - also, here the mapping appears to be done unconditionally, even if its result won't actually be used (because !isCBDependent) ... unless the compiler is smart enough to defer the computation until it's needed. The suggested change will avoid this potential redundancy.

@@ +2713,5 @@
> +  WritingMode wm = GetWritingMode();
> +  mozilla::css::Side inlineStart = wm.PhysicalSide(eLogicalSideIStart);
> +  mozilla::css::Side inlineEnd   = wm.PhysicalSide(eLogicalSideIEnd);
> +  mozilla::css::Side blockStart  = wm.PhysicalSide(eLogicalSideBStart);
> +  mozilla::css::Side blockEnd    = wm.PhysicalSide(eLogicalSideBEnd);

Ditto in this method.
Attachment #8599231 - Flags: review?(jfkthame) → review+
Comment on attachment 8599233 [details] [diff] [review]
Patch 7: use abstract coordinates in ComputeMinMaxValues

Review of attachment 8599233 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.cpp
@@ +2757,5 @@
>  {
> +  WritingMode wm = GetWritingMode();
> +
> +  const nsStyleCoord& minISize = wm.IsVertical()
> +    ? mStylePosition->mMinHeight : mStylePosition->mMinWidth;

More concise:

  const nsStyleCoord& minISize = mStylePosition->MinISize(wm);

etc.

@@ +2776,2 @@
>                                            mStylePosition->mBoxSizing,
> +                                          minISize);

pre-existing nit: perhaps fix alignment of the parameters while you're here? Likewise in several following calls.
Attachment #8599233 - Flags: review?(jfkthame) → review+
Comment on attachment 8599690 [details] [diff] [review]
Patch 8: use logical border and padding in nsHTMLReflowState::GetHypotheticalBoxContainer

Review of attachment 8599690 [details] [diff] [review]:
-----------------------------------------------------------------

OK for now, though bug 1079151 will modify this again. :)
Attachment #8599690 - Flags: review?(jfkthame) → review+
Comment on attachment 8608766 [details] [diff] [review]
Patch 10: Use a single LogicalSize argument in nsHTMLReflowState instead of aContainingBlockWidth and aContainingBlockHeight

Review of attachment 8608766 [details] [diff] [review]:
-----------------------------------------------------------------

feedback+, as I think this is the direction we want to go, but I assume an updated patch is coming.
Attachment #8608766 - Flags: feedback?(jfkthame) → feedback+
Blocks: 1079151
Updated to comments, carrying forward r=jfkthame
Attachment #8583784 - Attachment is obsolete: true
Attachment #8615193 - Flags: review+
Carrying forward r=jfkthame
Attachment #8615198 - Flags: review+
(I still have the individual patches around in another tree if you think the folding together was to enthusiastic)
Attachment #8584309 - Attachment is obsolete: true
Attachment #8599173 - Attachment is obsolete: true
Attachment #8599231 - Attachment is obsolete: true
Attachment #8599233 - Attachment is obsolete: true
Attachment #8599690 - Attachment is obsolete: true
Attachment #8608760 - Attachment is obsolete: true
Attachment #8608763 - Attachment is obsolete: true
Attachment #8608766 - Attachment is obsolete: true
Attachment #8609999 - Attachment is obsolete: true
Attachment #8615193 - Attachment is obsolete: true
Attachment #8609999 - Flags: feedback?(smontagu)
Attachment #8615245 - Flags: review+
Attachment #8584310 - Attachment is obsolete: true
Blocks: 1183439
You need to log in before you can comment on or make changes to this bug.