1079154 - Logical coordinate support for relative positioning

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

Part of support for vertical text

Comment 1

[Simon Montagu :smontagu]

I'm not clear what the desirable and/or specified behaviour is here.  http://dev.w3.org/csswg/css-writing-modes/#vertical-layout includes "positioning offsets" among the properties that use flow-relative directions, but on the other hand "the side of the box these properties apply to doesn’t change: only which values are inputs to which layout calculations changes".

I don't know where that leaves us here, nor what code needs to change how.

Comment 2

[Simon Montagu :smontagu]

Attachment: w-i-p patch 1 - ApplyRelativePositioning

Attaching this w-i-p before I go on vacation. Other callsites also need to be fixed.

Comment 3

[Simon Montagu :smontagu]

Attachment: w-i-p patch 2 - ReflowChild

Another w-i-p patch, again without all callsites patched.

Comment 4

[Simon Montagu :smontagu]

Attachment: patch 1: add a != operator to LogicalPoint and sprinkle some const dust in WritingModes.h

Comment 5

[Simon Montagu :smontagu]

Attachment: patch 2: add SetPhysicalPosition to LogicalRect and SetPosition with a LogicalPoint to nsIFrame

Comment 6

[Simon Montagu :smontagu]

Attachment: patch 3: logical versions of ApplyRelativePositioning

Comment 7

[Simon Montagu :smontagu]

Attachment: patch 4: Logical versions of ReflowChild and FinishReflowChild

Comment 8

[Simon Montagu :smontagu]

Attachment: patch 5: convert callers of ApplyRelativePosition, ReflowChild and FinishReflowChild in layout/generic to the logical versions

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8538625 [details] [diff] [review]
patch 1: add a != operator to LogicalPoint and sprinkle some const dust in WritingModes.h

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

::: layout/generic/WritingModes.h
@@ +561,5 @@
>                             GetPhysicalPoint(aFromMode, aContainerWidth),
>                             aContainerWidth);
>    }
>  
> +  bool operator==(const LogicalPoint aOther) const

I've been wondering - should we pass LogicalPoints by const reference, like we typically do for larger structs? Especially for 32-bit processors (low-end mobile systems?), it might be preferable. WDYT?

As long as we're passing by value, the param doesn't really need to be constified here, though I suppose it doesn't hurt.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8538626 [details] [diff] [review]
patch 2: add SetPhysicalPosition to LogicalRect and SetPosition with a LogicalPoint to nsIFrame

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

I have an experimental patch that switches the actual storage of nsIFrame::mRect from physical to logical; I wonder whether that would be beneficial, on balance. But we can think about that in due course.... this looks OK for now.

Comment 11

[Simon Montagu :smontagu]

Try push for this patch set (including reftest for bug 1102175) https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3559fef6127

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8538627 [details] [diff] [review]
patch 3: logical versions of ApplyRelativePositioning

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

Clearing r? on this version of the patch: I think this can be simplified as discussed below. (And if so, the preceding patch can be updated similarly.)

::: layout/generic/nsHTMLReflowState.h
@@ +825,5 @@
> +                           mozilla::LogicalPoint* aPosition,
> +                           nscoord aContainerWidth) {
> +    mozilla::LogicalRect rect(aWritingMode, *aPosition,
> +                              aFrame->GetLogicalSize(aWritingMode));
> +    nsPoint pos = rect.GetPhysicalPosition(aWritingMode, aContainerWidth);

The same idiom of converting a position for a rect from physical to logical via a temporary LogicalRect, occurred in the previous patch, too; should we have a convenience method for this? I'm not sure what to call it, though...

Maybe a new method on LogicalPoint, like GetPhysicalPoint but with an additional param for the rect width:

  nsPoint GetPhysicalPositionForRectWidth(nscoord aRectWidth,
                                          WritingMode aWritingMode,
                                          nscoord aContainerWidth);

This calls out the fact that it's the width of the rect that makes this potentially different from a simple GetPhysicalPoint conversion of the given position. That wouldn't be valid because RTL rects (either horizontal-rtl or vertical-rl) have their origin at the top right instead of the top left.

In fact (thinking as I write, here...), can't we do this more cleanly by simply subtracting the width of the rect from the container-width that we pass to GetPhysicalPoint()? I.e., I think these two lines can be replaced by

  nscoord frameWidth = aFrame->GetSize().width;
  nsPoint pos = aPosition->GetPhysicalPoint(aWritingMode,
                                            aContainerWidth - frameWidth);

I think this makes sense: we're just doing a standard logical/physical point conversion, with the added twist that if we're doing a horizontal "reflection" within a container width, we need to adjust this by the width of the target frame to account for the origin switching sides. Does this work?

@@ +830,5 @@
> +    ApplyRelativePositioning(aFrame,
> +                             aComputedOffsets.GetPhysicalMargin(aWritingMode),
> +                             &pos);
> +    rect.SetPhysicalPosition(aWritingMode, pos, aContainerWidth);
> +    *aPosition = rect.Origin(aWritingMode);

And in view of the comment above, I think we can replace this with

  *aPosition = mozilla::LogicalPoint(aWritingMode, pos,
                                     aContainerWidth - frameWidth);

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8538626 [details] [diff] [review]
patch 2: add SetPhysicalPosition to LogicalRect and SetPosition with a LogicalPoint to nsIFrame

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

I have an experimental patch that switches the actual storage of nsIFrame::mRect from physical to logical; I wonder whether that would be beneficial, on balance. But we can think about that in due course.... this looks OK for now.

::: layout/generic/nsIFrame.h
@@ +737,5 @@
> +  void SetPosition(mozilla::WritingMode aWritingMode,
> +                   mozilla::LogicalPoint aPt,
> +                   nscoord aContainerWidth) {
> +    mozilla::LogicalRect rect(aWritingMode, aPt, GetLogicalSize(aWritingMode));
> +    mRect.MoveTo(rect.GetPhysicalPosition(aWritingMode, aContainerWidth));

After thinking about patch 3, I propose

  // We subtract mRect.width from the container width to account for the
  // fact that logical origins in RTL coordinate systems are at the top right
  // of the frame instead of the top left.
  mRect.MoveTo(aPt.GetPhysicalPoint(aWritingMode, aContainerWidth - mRect.width));

Clearing review flag pending consideration of this option.

Comment 14

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> I've been wondering - should we pass LogicalPoints by const reference, like
> we typically do for larger structs? Especially for 32-bit processors
> (low-end mobile systems?), it might be preferable. WDYT?

As far as I can see we already do pass them by const reference everywhere in WritingMode.h except for operator==. I'll change == and != to do the same in this patch before checking in.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8538629 [details] [diff] [review]
patch 5: convert callers of ApplyRelativePosition, ReflowChild and FinishReflowChild in layout/generic to the logical versions

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

::: layout/generic/nsBlockReflowContext.cpp
@@ +429,5 @@
> +    LogicalRect(mWritingMode, mICoord, mBCoord,
> +                mMetrics.ISize(mWritingMode),
> +                mMetrics.BSize(mWritingMode)).ConvertTo(frameWM, mWritingMode,
> +                                                        mContainerWidth);
> +  LogicalPoint logPos = frameRect.Origin(frameWM);

I think this should work (untested, by analogy with comments on earlier patches):

  LogicalPoint logPos =
    LogicalPoint(mWritingMode, mICoord, mBCoord).
      ConvertTo(frameWM, mWritingMode, mContainerWidth - mMetrics.Width());

::: layout/generic/nsColumnSetFrame.cpp
@@ +484,5 @@
>  
>    nsIFrame* child = mFrames.FirstChild();
> +  LogicalPoint childOrigin(wm, borderPadding.IStart(wm),
> +                           borderPadding.BStart(wm));
> +  nscoord containerWidth = aReflowState.ComputedWidth();

In the vertical-rl case, this is likely to be UNCONSTRAINED at this point; so then using this value in the MoveChildTo call below will throw the columns out of view to the right. :(

This feels rather like the problem we had with block elements within a vertical-rl div (bug 1088025). Until we've reflowed the children, we don't know the final block-size of their parent; but that's what we need as their container width.

(Sorry, I haven't put together a vertical-rl columns reftest to automatically catch this yet. But it definitely breaks.)

There's already a fragment of code that adjusts the positions of child frames in the vertical-rl case after we've reflowed them all. Maybe this can be updated to handle this?

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8538628 [details] [diff] [review]
patch 4: Logical versions of ReflowChild and FinishReflowChild

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

::: layout/generic/nsContainerFrame.cpp
@@ +927,5 @@
> +                              uint32_t                 aFlags,
> +                              nsReflowStatus&          aStatus,
> +                              nsOverflowContinuationTracker* aTracker)
> +{
> +  NS_PRECONDITION(aReflowState.frame == aKidFrame, "bad reflow state");

Can we assert that aContainerWidth is not UNCONSTRAINED here, and similarly in FinishReflowChild? That might help us catch misuse of these methods.

@@ +1089,5 @@
> +  }
> +
> +  nsPoint newOrigin = aKidFrame->GetPosition();
> +  if (!(aFlags & NS_FRAME_NO_MOVE_VIEW) &&
> +      (curOrigin.x != newOrigin.x || curOrigin.y != newOrigin.y)) {

Can't we just say (curOrigin != newOrigin) here?

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for column child-frame positioning in vertical-rl mode

Here's a reftest that demonstrates the problem with positioning the child frames of a column set. This passes with current trunk, but will fail with the current version of patch 5 here.

Comment 18

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Comment on attachment 8538628 [details] [diff] [review]
> patch 4: Logical versions of ReflowChild and FinishReflowChild
> 
> Can we assert that aContainerWidth is not UNCONSTRAINED here, and similarly
> in FinishReflowChild? That might help us catch misuse of these methods.

We can, but the assertion will fire in some stress-testy crashtests that deliberately set huge widths -- see https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dfc944ba087. We will have to either annotate the extra assertions or make it a warning instead.

Comment 19

[Simon Montagu :smontagu]

Attachment: patch 1 v.2: add a != operator to LogicalPoint and sprinkle some const dust in WritingModes.h

Changed to pass LogicalPoints by const ref per comment 9

Transferring r=jfkthame

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #18)
> (In reply to Jonathan Kew (:jfkthame) from comment #16)
> > Comment on attachment 8538628 [details] [diff] [review]
> > patch 4: Logical versions of ReflowChild and FinishReflowChild
> > 
> > Can we assert that aContainerWidth is not UNCONSTRAINED here, and similarly
> > in FinishReflowChild? That might help us catch misuse of these methods.
> 
> We can, but the assertion will fire in some stress-testy crashtests that
> deliberately set huge widths -- see
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dfc944ba087. We
> will have to either annotate the extra assertions or make it a warning
> instead.

Looks like only a couple of crashtests are affected; given that, I think it'd be worth having the assertion here and annotating the tests that expect it. Debug builds are often so console-spammy from a variety of sources that a warning here may be too easy to overlook. :(

Comment 21

[Simon Montagu :smontagu]

Attachment: patch 2 v.2: add SetPosition with a LogicalPoint to nsIFrame

I like the suggestion in comments 12 and 13 a lot: it makes everything simpler and it means not only don't we need SetPhysicalPosition any more, we will also be able to get rid of GetPhysicalPosition in a follow-up patch. (You will have noticed that I incorporated this pattern into the fix for bug 1113526 too!)

(In reply to Jonathan Kew (:jfkthame) from comment #10)

> I have an experimental patch that switches the actual storage of
> nsIFrame::mRect from physical to logical; I wonder whether that would be
> beneficial, on balance.

My gut feeling is that we would still end up doing more conversions that way than if the storage is physical, but I'm more than open to benchmarking it and making the change if it's a win.

Comment 22

[Simon Montagu :smontagu]

Attachment: patch 3: logical versions of ApplyRelativePositioning

Again, adopting the suggestion from comment 12

Comment 23

[Simon Montagu :smontagu]

Attachment: patch 4 v.2: Logical versions of ReflowChild and FinishReflowChild

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> > +      (curOrigin.x != newOrigin.x || curOrigin.y != newOrigin.y)) {
> 
> Can't we just say (curOrigin != newOrigin) here?

Um, good point. (I'll do the assertion or warning, whichever we decide on, in a separate patch)

Comment 24

[Simon Montagu :smontagu]

Attachment: patch 5 v.2: convert callers of ApplyRelativePosition, ReflowChild and FinishReflowChild in layout/generic to the logical versions

Updated to comment 15. The original code for adjusting columns in vertical-rl mode is no longer necessary now that we're using logical coordinates in the main loop, so it's just replaced in this version with a new loop that does a different calculation with the post-reflow container width.

Comment 25

[Simon Montagu :smontagu]

Attachment: Patch 6: convert ApplyRelativePosition, ReflowChild and FinishReflowChild in nsFlexContainerFrame to the logical versions

This is a bit clunky but is probably the best we can do until flexboxes are updated to use writing-mode throughout -- maybe it can wait for the time being - WDYT?

Comment 26

[Simon Montagu :smontagu]

Attachment: Patch 7: remove obsoleted LogicalRect::GetPhysicalPosition

As I said above, this is no longer needed.

Comment 27

[Simon Montagu :smontagu]

Attachment: Patch 4a: add assertions for UNCONSTRAINED width to ReflowChild and FinishReflowChild and annotate crashtests as necessary

Comment 28

[Simon Montagu :smontagu]

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

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8540352 [details] [diff] [review]
patch 2 v.2: add SetPosition with a LogicalPoint to nsIFrame

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

::: layout/generic/nsIFrame.h
@@ +726,5 @@
>      SetRect(nsRect(mRect.TopLeft(), aSize));
>    }
>    void SetPosition(const nsPoint& aPt) { mRect.MoveTo(aPt); }
> +  void SetPosition(mozilla::WritingMode aWritingMode,
> +                   mozilla::LogicalPoint aPt,

pass aPt as const reference

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 8540361 [details] [diff] [review]
patch 4 v.2: Logical versions of ReflowChild and FinishReflowChild

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

::: layout/generic/nsContainerFrame.cpp
@@ +921,5 @@
>                                nsPresContext*           aPresContext,
>                                nsHTMLReflowMetrics&     aDesiredSize,
>                                const nsHTMLReflowState& aReflowState,
> +                              WritingMode              aWM,
> +                              LogicalPoint             aPos,

const reference

@@ +1063,5 @@
>                                      nsPresContext*             aPresContext,
>                                      const nsHTMLReflowMetrics& aDesiredSize,
>                                      const nsHTMLReflowState*   aReflowState,
> +                                    WritingMode                aWM,
> +                                    LogicalPoint               aPos,

const reference

::: layout/generic/nsContainerFrame.h
@@ +245,5 @@
>                     nsPresContext*                 aPresContext,
>                     nsHTMLReflowMetrics&           aDesiredSize,
>                     const nsHTMLReflowState&       aReflowState,
> +                   mozilla::WritingMode           aWM,
> +                   mozilla::LogicalPoint          aPos,

const reference

@@ +273,5 @@
>                                  nsPresContext*             aPresContext,
>                                  const nsHTMLReflowMetrics& aDesiredSize,
>                                  const nsHTMLReflowState*   aReflowState,
> +                                mozilla::WritingMode       aWM,
> +                                mozilla::LogicalPoint      aPos,

const reference

Comment 31

[Jonathan Kew [:jfkthame]]

Comment on attachment 8540374 [details] [diff] [review]
Patch 4a: add assertions for UNCONSTRAINED width to ReflowChild and FinishReflowChild and annotate crashtests as necessary

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

::: layout/generic/nsContainerFrame.cpp
@@ +929,5 @@
>                                nsOverflowContinuationTracker* aTracker)
>  {
>    NS_PRECONDITION(aReflowState.frame == aKidFrame, "bad reflow state");
> +  NS_ASSERTION(aContainerWidth != NS_UNCONSTRAINEDSIZE,
> +               "ReflowChild with unconstrained width!");

make this "...unconstrained container-width", please

@@ +1070,5 @@
>                                      nscoord                    aContainerWidth,
>                                      uint32_t                   aFlags)
>  {
> +  NS_ASSERTION(aContainerWidth != NS_UNCONSTRAINEDSIZE,
> +               "FinishReflowChild with unconstrained width!");

ditto

Comment 32

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #28)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8884f915af0f

Hmm, tryserver says some adjustment is needed to those annotations.... but more worryingly, that layout/generic/crashtests/421671.html is having a really bad time on android. :( Maybe we need to hold off on adding that assertion.

Comment 33

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #32)

> Hmm, tryserver says some adjustment is needed to those annotations.... but
> more worryingly, that layout/generic/crashtests/421671.html is having a
> really bad time on android. :( Maybe we need to hold off on adding that
> assertion.

Yes, I've made the necessary adjustments, but I'm not sure what to do about those 6460 assertions on 421671.html on Android! I rather doubt if it's even a constant number, what with the <marquee> in the test and all. Cutting the Gordian know with "skip-if(Android)" is tempting...

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This might require a little bit of merging with bug 35168.

Comment 35

[Simon Montagu :smontagu]

On reflection I think we do need to hold off on the assertion, or at least limit it a bit: as it is it fires in vertical-lr mode as well, where the aContainerWidth (coming from aReflowState.ComputedWidth()) is indeed NS_UNCONSTRAINEDSIZE, but who cares because we never use it in vertical-lr mode anyway. I'll see how limiting it to right-to-left modes affects tryserver.

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8540367 [details] [diff] [review]
Patch 6: convert ApplyRelativePosition, ReflowChild and FinishReflowChild in nsFlexContainerFrame to the logical versions


>+++ b/layout/generic/nsFlexContainerFrame.cpp
>@@ -3603,31 +3603,38 @@ nsFlexContainerFrame::DoFlexLayout(nsPre
>+  WritingMode outerWM = aReflowState.GetWritingMode();
>+  nscoord containerWidth = aReflowState.ComputedWidth();

We probably shouldn't be re-getting aReflowState.ComputedWidth() here. The container's dimensions are aContentBoxMainSize & contentBoxCrossSize (the sizes we pass to "PhysicalPointFromLogicalPoint")  (btw, as you probably noticed, that's "logical point" in flex space, not a real LogicalPoint... I need to rename those methods now I guess).

In practice, I think the widthy dimension does *turn out* to be aReflowState.ComputedWidth(), but I'd rather not rely on that.

So -- I'd prefer:
   nscoord containerWidth = IsAxisHorizontal(mMainAxis) ?
     aContentBoxMainSize : contentBoxCrossSize;

This will hopefully be cleaner once we resolve your XXX comment and resolve the double-conversion.

r=me with that

Comment 37

[Jonathan Kew [:jfkthame]]

Comment on attachment 8540365 [details] [diff] [review]
patch 5 v.2: convert callers of ApplyRelativePosition, ReflowChild and FinishReflowChild in layout/generic to the logical versions

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

Sorry, I thought I'd posted this, but I don't see it on the bug..... stuck as a Draft, apparently. :(

::: layout/generic/nsColumnSetFrame.cpp
@@ +490,5 @@
> +                           borderPadding.BStart(wm));
> +  // In vertical-rl mode we can't use the computed width as the
> +  // container width because it may be NS_UNCONSTRAINEDSIZE, so we use 0
> +  // for now and reposition the columns after reflowing them all.
> +  nscoord containerWidth = wm.IsVerticalRL() ? 0 : aReflowState.ComputedWidth();

Regardless of the writing mode, an unconstrained container-width can't be useful, can it? I don't think there's any obvious way to end up with NS_UNCONSTRAINEDSIZE in the horizontal case here (which might mess up RTL), but I'm not really confident of that.

Maybe it'd be worth adding an assertion here that containerWidth != NS_UNCONSTRAINEDSIZE, just in case?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +503,5 @@
>    mHelper.mHasVerticalScrollbar = aAssumeVScroll;
>  
>    nsReflowStatus status;
>    ReflowChild(mHelper.mScrolledFrame, presContext, *aMetrics,
> +              kidReflowState, wm, LogicalPoint(wm), 0,

What makes it OK to pass zero as container-width here? Assuming this is legitimate, it might be worth an explanatory comment.

@@ +515,5 @@
>    // resize here would size it to the natural height of the frame,
>    // which will usually be different from the scrollport height;
>    // invalidating the difference will cause unnecessary repainting.
>    FinishReflowChild(mHelper.mScrolledFrame, presContext,
> +                    *aMetrics, &kidReflowState, wm, LogicalPoint(wm), 0,

And here?

Comment 38

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #37)
> Maybe it'd be worth adding an assertion here that containerWidth !=
> NS_UNCONSTRAINEDSIZE, just in case?

Was this on the assumption that we wouldn't add the assertion to ReflowChild/FinishReflowChild? I don't think we need both. Also, asserting here would have the same problem that I mention in comment 35, that it would fire unnecessarily in vertical-lr.

> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +503,5 @@
> >    mHelper.mHasVerticalScrollbar = aAssumeVScroll;
> >  
> >    nsReflowStatus status;
> >    ReflowChild(mHelper.mScrolledFrame, presContext, *aMetrics,
> > +              kidReflowState, wm, LogicalPoint(wm), 0,
> 
> What makes it OK to pass zero as container-width here? Assuming this is
> legitimate, it might be worth an explanatory comment.

I've added a comment:

  // No need to pass a container-width to ReflowChild or
  // FinishReflowChild, because it's only used there when positioning
  // the frame (i.e. if NS_FRAME_NO_MOVE_FRAME isn't set)

Comment 39

[Simon Montagu :smontagu]

Attachment: Patch 4a v.2: add assertions for UNCONSTRAINED width to ReflowChild and FinishReflowChild, but only for right-to-left writing mode

With this version the extra annotations aren't needed. I'd still like to add these assertions: they have already helped me find errors in patches I've been working on for callers of (Finish)ReflowChild elsewhere in the tree.

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

Comment 40

[Simon Montagu :smontagu]

(In reply to Daniel Holbert [:dholbert] from comment #36)
> In practice, I think the widthy dimension does *turn out* to be
> aReflowState.ComputedWidth(), but I'd rather not rely on that.
> 
> So -- I'd prefer:
>    nscoord containerWidth = IsAxisHorizontal(mMainAxis) ?
>      aContentBoxMainSize : contentBoxCrossSize;

Hmm, I thought I'd done it this way originally and it caused regressions, but apparently not (or rather, they must have been caused by some other issue that I've fixed since).

Comment 41

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #38)
> (In reply to Jonathan Kew (:jfkthame) from comment #37)
> > Maybe it'd be worth adding an assertion here that containerWidth !=
> > NS_UNCONSTRAINEDSIZE, just in case?
> 
> Was this on the assumption that we wouldn't add the assertion to
> ReflowChild/FinishReflowChild? I don't think we need both. Also, asserting
> here would have the same problem that I mention in comment 35, that it would
> fire unnecessarily in vertical-lr.

OK, no need for it here then.


> I've added a comment:
> 
>   // No need to pass a container-width to ReflowChild or
>   // FinishReflowChild, because it's only used there when positioning
>   // the frame (i.e. if NS_FRAME_NO_MOVE_FRAME isn't set)

Makes sense, thanks.

Comment 42

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb4df421364
https://hg.mozilla.org/integration/mozilla-inbound/rev/537a87e2a74e
https://hg.mozilla.org/integration/mozilla-inbound/rev/679016653e34
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa61a0310e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c16d59bb79d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2d6f002fee
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a2cdfc45ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/547a2d626a62

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bdb4df421364
https://hg.mozilla.org/mozilla-central/rev/537a87e2a74e
https://hg.mozilla.org/mozilla-central/rev/679016653e34
https://hg.mozilla.org/mozilla-central/rev/1fa61a0310e7
https://hg.mozilla.org/mozilla-central/rev/c16d59bb79d1
https://hg.mozilla.org/mozilla-central/rev/0b2d6f002fee
https://hg.mozilla.org/mozilla-central/rev/d2a2cdfc45ec
https://hg.mozilla.org/mozilla-central/rev/547a2d626a62

Comment 44

[Fabien Cazenave [:kaze]]

This patch causes a regression (in particular for RTL content), see bug 1121748.

Comment 45

[Jonathan Kew [:jfkthame]]

Simon, can we close this now, given that bug 1121748 seems to be safely resolved, and file separate followups as required for any further work?

Comment 46

[Simon Montagu :smontagu]

Yes, let's do that