1147834 - Convert remaining uses of physical coordinates in nsHTMLReflowState

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Simon Montagu :smontagu]

Following bug 1130907 comment 3, we need to convert the remaining places in nsHTMLReflowState that don't use abstract coordinates.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch 1: initialize CSSOffsetState with the containing block inline size

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)

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch 2: rename ISize/BSize to width/height throughout nsHTMLReflowState

This is a rename-only patch to make other changes easier to follow.

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch 3: use abstract coordinates in ComputeRelativeOffsets

Comment 4

[Simon Montagu :smontagu]

Attachment: Tests for overconstrained relative positioning

These tests fail before patch 3 and pass after it.

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch 2: rename width/height to ISize/BSize throughout nsHTMLReflowState

Parts of this were obsoleted by bug 1156021

Comment 6

[Simon Montagu :smontagu]

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

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch 4: use abstract coordinates when calculating margins, borders and paddingpatch 4: use abstract coordinates when calculating margins, borders and padding

This causes an additional regression in layout/reftests/writing-mode/1102175-1b.html

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch 5: use logical border and padding when calculating containing block sizes

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

Comment 9

[Simon Montagu :smontagu]

Attachment: Patch 6: use inline and block size as the basis of percentage margin and padding sizes

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch 7: use abstract coordinates in ComputeMinMaxValues

This fixes the regression in layout/reftests/writing-mode/1158549-1-vertical-block-size-constraints.html

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch 8: use logical border and padding in nsHTMLReflowState::GetHypotheticalBoxContainer

Comment 12

[Jonathan Kew [:jfkthame]]

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 13

[Jonathan Kew [:jfkthame]]

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 14

[Jonathan Kew [:jfkthame]]

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 15

[Jonathan Kew [:jfkthame]]

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 16

[Jonathan Kew [:jfkthame]]

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 17

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd689f33201

Comment 18

[Jonathan Kew [:jfkthame]]

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.

Comment 19

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2665b22e8980

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: patch 9: followup to patch 4, use containing block's writing mode in CalculateHypotheticalBox

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).

Comment 21

[Simon Montagu :smontagu]

Attachment: Patch 4 updated to comments: use abstract coordinates when calculating margins, borders and padding

Comment 22

[Simon Montagu :smontagu]

Attachment: Patch 5 updated to comments: use logical border and padding when calculating containing block sizes

Comment 23

[Simon Montagu :smontagu]

Attachment: Patch 10: Use a single LogicalSize argument in nsHTMLReflowState instead of aContainingBlockWidth and aContainingBlockHeight

Comment 24

[Simon Montagu :smontagu]

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

Comment 25

[Simon Montagu :smontagu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8d3d94469df

Comment 26

[Simon Montagu :smontagu]

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

Comment 27

[Jonathan Kew [:jfkthame]]

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.)

Comment 28

[Jonathan Kew [:jfkthame]]

(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.

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: Followup to patch 10

Proposed fixes to patch 10.

Comment 30

[Jonathan Kew [:jfkthame]]

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 31

[Simon Montagu :smontagu]

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?

Comment 32

[Simon Montagu :smontagu]

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

Comment 33

[Jonathan Kew [: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.

Comment 34

[Simon Montagu :smontagu]

(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 35

[Jonathan Kew [:jfkthame]]

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 36

[Jonathan Kew [:jfkthame]]

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

Comment 37

[Jonathan Kew [:jfkthame]]

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.

Comment 38

[Jonathan Kew [:jfkthame]]

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/

Comment 39

[Jonathan Kew [:jfkthame]]

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

Comment 40

[Jonathan Kew [:jfkthame]]

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

I mean 1167930, of course.

Comment 41

[Jonathan Kew [:jfkthame]]

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.

Comment 42

[Jonathan Kew [:jfkthame]]

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.

Comment 43

[Jonathan Kew [:jfkthame]]

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. :)

Comment 44

[Jonathan Kew [:jfkthame]]

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.

Comment 45

[Simon Montagu :smontagu]

Attachment: Patch 1: initialize CSSOffsetState with the containing block inline size

Updated to comments, carrying forward r=jfkthame

Comment 46

[Simon Montagu :smontagu]

Attachment: Tests for overconstrained relative positioning, updated to comments

Carrying forward r=jfkthame

Comment 47

[Simon Montagu :smontagu]

Attachment: All patches folded together for checkin

(I still have the individual patches around in another tree if you think the folding together was to enthusiastic)

Comment 48

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0145b66ac03
https://hg.mozilla.org/integration/mozilla-inbound/rev/42874d398048

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0145b66ac03
https://hg.mozilla.org/mozilla-central/rev/42874d398048