Closed Bug 1131451 Opened 9 years ago Closed 9 years ago

Support vertical writing-modes with dir=rtl

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: smontagu, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, rtl)

Attachments

(11 files, 11 obsolete files)

5.64 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.68 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.57 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.51 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.92 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.94 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.12 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.13 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
258.12 KB, patch
Details | Diff | Splinter Review
2.73 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.87 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
In bug 1131013 we decided that bidi text in vertical writing modes is a sufficiently rare use case that it's not worth investing in for the initial release of writing-mode support. We should add it later though.

This will require taking the container height into account when calculating inline coordinates, just as we do now with container width for dir=rtl in horizontal writing modes.
Version: unspecified → Trunk
Depends on: 1167930
Fixing this will also fix things like bug 1180528, and the currently-failing testcases from bug 1150614. I've got some WIP locally that I hope will be ready soon.
Assignee: nobody → jfkthame
Blocks: 1180528
Depends on: 1177614
This is big but largely boring; the idea is simply to have containerSize (i.e. two-dimensional) wherever we currently have containerWidth (horizontal-only). This patch just makes us pass around containerSizes, but does not make use of the height for y-coordinate conversions, so it won't actually change behavior yet (that's the next patch).
Attachment #8631663 - Flags: review?(dholbert)
Here, we use the containerSize's height to fix the inline-coordinate conversions for dir=rtl in vertical writing modes. This will cause breakage in places where we're currently doing ad-hoc workarounds, so we'll need to remove those; that will be the following series of patches (which should all be folded into this one for landing, to avoid broken intermediate stages).
Attachment #8631666 - Flags: review?(dholbert)
(Heads-up, one of the chunks in "part 1" here was bitrotted (slightly) by another patch of yours in bug 1175094, in a way that can't be hacked around w/ patch -F.

Not causing problems on my end; just letting you know since that'll need fixing before this lands. You may already have discovered & fixed this locally.)
(In reply to Daniel Holbert [:dholbert] from comment #13)
> (Heads-up, one of the chunks in "part 1" here was bitrotted (slightly) by
> another patch of yours in bug 1175094, in a way that can't be hacked around
> w/ patch -F.
> 
> Not causing problems on my end; just letting you know since that'll need
> fixing before this lands. You may already have discovered & fixed this
> locally.)

Yes, thanks. It'll also be slightly bitrotted by bug 1181890, which I guess is likely to land soon. But the updates needed should be trivial.
Comment on attachment 8631663 [details] [diff] [review]
part 1 - Replace containerWidth with containerSize in logical-coordinate classes and APIs, frame classes, etc

First review note (affects various places in this patch):

>+++ b/layout/forms/nsComboboxControlFrame.cpp
>@@ -465,22 +465,22 @@ nsComboboxControlFrame::ReflowDropdown(n
>   // That would be odd!
>   // Note that we don't need to pass the true frame position or container width
>   // to ReflowChild or FinishReflowChild here; it will be positioned as needed
>   // by AbsolutelyPositionDropDown().
>   WritingMode outerWM = GetWritingMode();
>   nsHTMLReflowMetrics desiredSize(aReflowState);
>   nsReflowStatus ignoredStatus;
>   ReflowChild(mDropdownFrame, aPresContext, desiredSize,
>-              kidReflowState, outerWM, LogicalPoint(outerWM), 0,
>+              kidReflowState, outerWM, LogicalPoint(outerWM), nsSize(),
>               flags, ignoredStatus);
> 
>    // Set the child's width and height to its desired size
>   FinishReflowChild(mDropdownFrame, aPresContext, desiredSize, &kidReflowState,
>-                    outerWM, LogicalPoint(outerWM), 0, flags);
>+                    outerWM, LogicalPoint(outerWM), nsSize(), flags);

Two things:
 (1) The comment needs s/container width/container size/

 (2) I'm a bit uncomfortable with passing a "nsSize()" arg like this -- it's a bit mysterious/magical, and it obscures the fact that we're kinda cheating. I'd rather this code call use a usefully-named local variable, like so:

  const nsSize dummyContainerSize; // Don't need real size because $FOO.

  ReflowChild(mDropdownFrame, aPresContext, desiredSize,
              kidReflowState, outerWM, LogicalPoint(outerWM),
              dummyContainerSize, flags, ignoredStatus);

  // Set the child's width and height to its desired size
  FinishReflowChild(mDropdownFrame, aPresContext, desiredSize, &kidReflowState,
                    outerWM, LogicalPoint(outerWM), dummyContainerSize, flags);

This makes it clearer:
 - what this mysterious "nsSize()" arg represents.
 - that we're *intentionally* passing in bogus data.
 - why this is OK [via the comment - which we already have here]

(If you'd like, I'd be OK with this change happening via a followup, so that this first patch here can stay search-and-replacey. The followup should catch all of the "nsSize()" arg-passings in this patch; I count 27 of them w/ grep, though a lot of them are probably in the same sections of code & can use the same dummy local variable.)
(In reply to Daniel Holbert [:dholbert] from comment #15)
> I'd rather this code call use a usefully-named local variable, like so:
> 
>   const nsSize dummyContainerSize; // Don't need real size because $FOO.

(To clarify - this comment probably isn't needed in this particular spot, because the comment above already mentions why we don't need to pass in the container size.  My point was that there should be a brief explanatory comment wherever we need this pattern.)
Comment on attachment 8631663 [details] [diff] [review]
part 1 - Replace containerWidth with containerSize in logical-coordinate classes and APIs, frame classes, etc

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

part 1 review notes, wave 2 (through nsFieldSetFrame.cpp):

::: layout/forms/nsComboboxControlFrame.cpp
@@ +574,5 @@
>  
>    // Normal frame geometry (eg GetOffsetTo, mRect) doesn't include transforms.
>    // In the special case that our transform is only a 2D translation we
>    // introduce this hack so that the dropdown will show up in the right place.
> +  *aTranslation = LogicalPoint(aWM, GetCSSTransformTranslation(), nsSize());

(I suspect the reason we're passing nsSize() here because this represents a translation-vector, rather than a point inside of a box -- so no invert-and-subtracting is needed? I think this makes sense. If you like, an explanatory code-comment might help clarify that.)

@@ +866,5 @@
>  
>    // The button should occupy the same space as a scrollbar
>    WritingMode wm = aReflowState.GetWritingMode();
> +  nsSize containerSize = aReflowState.ComputedPhysicalSize();
> +  LogicalRect buttonRect = mButtonFrame->GetLogicalRect(containerSize);

(This is the piece that changed to include border-padding in bug 1175094.)

This should really be using your new utility method from bug 1177614 now.

::: layout/forms/nsFieldSetFrame.cpp
@@ +275,3 @@
>      clipRect.BStart(wm) += legendBorderWidth;
>      clipRect.BSize(wm) =
> +      GetLogicalRect(rect.Size()).BSize(wm) - (off + legendBorderWidth);

I think we should drop "GetLogicalRect(rect.Size())." from this line.

Explanation: "GetLogicalRect(rect.Size())" is wrong -- it's misusing 'rect'. This 'rect' variable is our *own* frame rect, basically, which we use above for positioning e.g. the legend frame.  But here, we're using it for in a call to this->GetLogicalRect, and that's not correct because it's not *our* container's size.  Fortunately, this GetLogicalRect call looks completely unnecessary -- we can just call "BSize(wm)" directly.

@@ +531,2 @@
>  
>      // update the container width after reflowing the inner frame

I think this comment needs s/width/size/? (and maybe a rewording to be clearer)

Pretty sure this refers to a FinishReflowChild argument, 2 lines down, which you're changing from
  containerWidth + kidDesiredSize.Width()
to
  containerSize + kidDesiredSize.PhysicalSize()

@@ +542,5 @@
>  
>    LogicalRect contentRect(wm);
>    if (inner) {
>      // We don't support margins on inner, so our content rect is just the
>      // inner's border-box. We don't care about container-width at this point,

s/container-width/container size/ (similar to my prev. comment)
Comment on attachment 8631663 [details] [diff] [review]
part 1 - Replace containerWidth with containerSize in logical-coordinate classes and APIs, frame classes, etc

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

Review notes, wave 3 (through nsBlockReflowState.h):

::: layout/forms/nsHTMLButtonControlFrame.cpp
@@ +319,5 @@
>    nsHTMLReflowMetrics contentsDesiredSize(aButtonReflowState);
>    childPos.B(wm) = 0; // This will be set properly later, after reflowing the
>                        // child to determine its size.
>  
> +  // We just pass 0 for containerSize here, as the child will be repositioned

This comment-tweak isn't quite sufficient -- technically needs s/0/"nsSize()"/. (But per comment 15, I'd really prefer we use a named dummy-variable instead of nsSize()...)

::: layout/generic/TextOverflow.cpp
@@ +262,5 @@
>  TextOverflow::TextOverflow(nsDisplayListBuilder* aBuilder,
>                             nsIFrame* aBlockFrame)
>    : mContentArea(aBlockFrame->GetWritingMode(),
>                   aBlockFrame->GetContentRectRelativeToSelf(),
> +                 aBlockFrame->GetRect().Size())

Just use aBlockFrame->GetSize() here. (no need to call GetRect)

@@ +267,4 @@
>    , mBuilder(aBuilder)
>    , mBlock(aBlockFrame)
>    , mScrollableFrame(nsLayoutUtils::GetScrollableFrameFor(aBlockFrame))
> +  , mBlockSize(aBlockFrame->GetRect().Size())

Here too.

::: layout/generic/WritingModes.h
@@ +640,5 @@
>        mPoint(aI, aB)
>    { }
>  
>    // Construct from a writing mode and a physical point, within a given
> +  // containing rectangle's size (defining the conversion between LTR

Right now this comment ends with "(defining the conversion between LTR and RTL coordinates)."
 
This needs to be broadened to mention ", and between top-to-bottom & bottom-to-top coordinates", or something like that.  (Though maybe this comment-tweak would belong in part 2, since that's where we actually start making that conversion.)

@@ +699,5 @@
>    {
>      CHECK_WRITING_MODE(aWritingMode);
>      if (aWritingMode.IsVertical()) {
> +      return nsPoint(aWritingMode.IsVerticalLR()
> +                       ? B() : aContainerSize.width - B(),

Coding-style guide says the "?" should be de-indented here, to be in same column as "aWritingMode":
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

(As a sanity-check: this is also what emacs automatically does if I hit "tab" to autoindent this line.)

@@ +704,4 @@
>                       I());
>      } else {
> +      return nsPoint(aWritingMode.IsBidiLTR()
> +                       ? I() : aContainerSize.width - I(),

Here too. (de-indent "?")

@@ +1377,5 @@
>  #endif
>    {
>      if (aWritingMode.IsVertical()) {
> +      mRect.y = aWritingMode.IsVerticalLR()
> +                  ? aRect.x : aContainerSize.width - aRect.XMost();

Here too. (de-indent "?")

@@ +1384,4 @@
>        mRect.width = aRect.height;
>      } else {
> +      mRect.x = aWritingMode.IsBidiLTR()
> +                  ? aRect.x : aContainerSize.width - aRect.XMost();

Here too. (de-indent "?")

@@ +1642,5 @@
>    {
>      CHECK_WRITING_MODE(aWritingMode);
>      if (aWritingMode.IsVertical()) {
> +      return nsRect(aWritingMode.IsVerticalLR()
> +                      ? BStart() : aContainerSize.width - BEnd(),

Here too. (de-indent "?")

@@ +1649,3 @@
>      } else {
> +      return nsRect(aWritingMode.IsBidiLTR()
> +                      ? IStart() : aContainerSize.width - IEnd(),

Here too. (de-indent "?")

::: layout/generic/nsBlockFrame.cpp
@@ +1281,3 @@
>        }
>        for (nsIFrame* f : mFloats) {
> +        nsPoint physicalDelta(deltaSize.width, 0*deltaSize.height);

What's the point of the 0*deltaSize.height here? (Why isn't this just either 0 or deltaSize.height?)

@@ +1284,5 @@
>          f->MovePositionBy(physicalDelta);
>        }
>        nsFrameList* bulletList = GetOutsideBulletList();
>        if (bulletList) {
> +        nsPoint physicalDelta(deltaSize.width, 0*deltaSize.height);

(here too, 0*deltaSize.height )

@@ +2836,2 @@
>  
>    // Changing container width only matters if writing mode is vertical-rl

Comment needs s/width/size/. (This is in UpdateLineContainerSize, which is being renamed from UpdateLineContainerWidth)

(I think the rest of the comment is still valid.)

::: layout/generic/nsBlockReflowState.cpp
@@ +857,5 @@
>      // with rtl direction. Since they don't yet (bug 1131451), we'll
>      // just put left floats at the top of the line and right floats at
>      // bottom.
> +    floatPos.I(wm) =
> +      leftFloat ? floatAvailableSpace.mRect.Y(wm, ContainerSize().height)

There's a code-comment right above this change, "IStart and IEnd should use the ContainerHeight in vertical modes", with a reference to this bug's number (bug 1131451).

I think that comment might be made stale by this change (?) and need removing. (Probably worth doing a cross-tree search for mentions of this bug number, if you haven't already done that.)
I think this addresses the review notes above, except for a couple that are handled in later patches here (because the behaviors aren't actually fixed yet at this stage).
Attachment #8633434 - Flags: review?(dholbert)
Attachment #8631663 - Attachment is obsolete: true
Attachment #8631663 - Flags: review?(dholbert)
Attachment #8631666 - Attachment is obsolete: true
Attachment #8631666 - Flags: review?(dholbert)
Attachment #8631667 - Attachment is obsolete: true
Attachment #8631667 - Flags: review?(dholbert)
Attachment #8631668 - Attachment is obsolete: true
Attachment #8631668 - Flags: review?(dholbert)
Attachment #8631669 - Attachment is obsolete: true
Attachment #8631669 - Flags: review?(dholbert)
Attachment #8631670 - Attachment is obsolete: true
Attachment #8631670 - Flags: review?(dholbert)
Attachment #8631671 - Attachment is obsolete: true
Attachment #8631671 - Flags: review?(dholbert)
Attachment #8633440 - Attachment is obsolete: true
Attachment #8633440 - Flags: review?(dholbert)
Attachment #8631672 - Attachment is obsolete: true
Attachment #8631672 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #18)

> ::: layout/generic/nsBlockReflowState.cpp
> @@ +857,5 @@
> >      // with rtl direction. Since they don't yet (bug 1131451), we'll
> >      // just put left floats at the top of the line and right floats at
> >      // bottom.
> > +    floatPos.I(wm) =
> > +      leftFloat ? floatAvailableSpace.mRect.Y(wm, ContainerSize().height)
> 
> There's a code-comment right above this change, "IStart and IEnd should use
> the ContainerHeight in vertical modes", with a reference to this bug's
> number (bug 1131451).
> 
> I think that comment might be made stale by this change (?) and need
> removing.

Not yet, as that doesn't get fixed until later. (The comment is removed in 2e.)

> (Probably worth doing a cross-tree search for mentions of this bug
> number, if you haven't already done that.)

The last mentions were on the couple of failing flexbox-writingmode reftests, now fixed by part 2g.

All the "part 2*" patches should be folded together before landing; I've posted them separately for now as they address separate parts of the code, but they're all fallout from making the logical-coord classes respect container height (the main "part 2"), and a number of them are required to prevent that breaking tests that currently pass due to our ad hoc workarounds in other parts of the code.
Comment on attachment 8633434 [details] [diff] [review]
part 1 - Replace containerWidth with containerSize in logical-coordinate classes and APIs, frame classes, etc

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

Thanks for addressing the first wave of review comments -- the dummyContainerSize stuff in particular makes this much easier to grok, IMO. Much appreciated!

r=me on part 1 with the following addressed:

::: layout/forms/nsFieldSetFrame.cpp
@@ +494,5 @@
>  
> +  // This containerSize is incomplete as yet: it does not include the size
> +  // of the |inner| frame itself.
> +  nsSize containerSize = (LogicalSize(wm, 0, mLegendSpace) +
> +                           border.Size(wm)).GetPhysicalSize(wm);

'border' should be de-indented 1 space here.

::: layout/generic/nsBlockFrame.cpp
@@ +1259,5 @@
>    ComputeFinalSize(*reflowState, state, aMetrics, &blockEndEdgeOfChildren);
>  
>    // If the block direction is right-to-left, we need to update the bounds of
> +  // lines that were placed relative to mContainerSize during reflow, as
> +  // we typically do not know the true container size (block-dir size of the

The "(block-dir size...)" parenthetical here is stale now -- it was a
parenthetical for "container width", but this doesn't say "container
width" anymore.

Just drop the parenthetical, I think -- looks like this still makes
sense without it. (Or reword as you see fit.)

@@ +2175,5 @@
>        // line dirty.
>        PropagateFloatDamage(aState, line, deltaBCoord);
>      }
>  
> +    // If the container size has changed reset the mContainerSize. If the

s/changed reset the mContainerSize/changed, reset mContainerSize/

(Drop "the", and add comma after "changed".)

::: layout/generic/nsColumnSetFrame.cpp
@@ +488,5 @@
>    nsIFrame* child = mFrames.FirstChild();
>    LogicalPoint childOrigin(wm, borderPadding.IStart(wm),
>                             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

This comment in nsColumnSetFrame::ReflowChildren needs adjustment. (It's very specific to what we did w/ "width" before this patch, but now we're doing something tricker and in both dimensions.)

(See also my next comment about the code this comment is describing, too...)

@@ +491,5 @@
>    // 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.
> +  nsSize containerSize = aReflowState.ComputedPhysicalSize();
> +  if (wm.IsVerticalRL() || containerSize.width == NS_UNCONSTRAINEDSIZE) {

Do we actually *need* both parts of this condition, or can we drop one of them?

(Perhaps we should drop the first part and just keep the UNCONSTRAINEDSIZE check, for consistency with what we do for height right below this code?)

@@ +809,5 @@
>    aDesiredSize.UnionOverflowAreasWithDesiredBounds();
>  
>    // In vertical-rl mode, make a second pass to reposition the columns
>    // with the correct container width
>    if (wm.IsVerticalRL()) {

I think this wm.IsVerticalRL() check may no longer be correct -- it needs to match the conditions up at the beginning of this function (where we set up our possibly-bogus containerSize).

::: layout/generic/nsFlexContainerFrame.cpp
@@ +3827,5 @@
> +  nsSize containerSize;
> +  containerSize.width = aAxisTracker.IsMainAxisHorizontal() ?
> +                          aContentBoxMainSize : contentBoxCrossSize;
> +  containerSize.width +=
> +    aReflowState.ComputedPhysicalBorderPadding().LeftRight();

We need a line like this for containerSize.height + [...].TopBottom, too (so that containerSize is truly the border-box size of the flex container, in both dimensions.)

::: layout/generic/nsFloatManager.cpp
@@ +244,5 @@
>    // convert back from LineLeft/Right to IStart
>    nscoord inlineStart = aWM.IsVertical() || aWM.IsBidiLTR()
> +                        ? lineLeft - mLineLeft
> +                        : mLineLeft - lineRight +
> +                          LogicalSize(aWM, aContainerSize).Width(aWM);

Why the LogicalSize .Width dance here? Can't we just replace this whole last line with "aContainerSize.width"?

(aContainerSize is our container's physical size. Seems like you're converting it to logical, in a temporary, for no reason here. Maybe I'm missing something though.)

::: layout/generic/nsGridContainerFrame.cpp
@@ +1246,5 @@
>  {
>    WritingMode wm = aReflowState.GetWritingMode();
>    const LogicalPoint gridOrigin(aContentArea.Origin(wm));
> +  const nsSize containerSize = (aContentArea.Size(wm) +
> +    aReflowState.ComputedLogicalBorderPadding().Size(wm)).GetPhysicalSize(wm);

The extreme-de-indentation is wrong here on 2nd line, now that "aContentArea[...]" is parenthesized.

(Part of a parenthesized expression shouldn't be de-indented to the left of the open-paren).

Maybe add a newline after "="?

@@ +1285,5 @@
>      LogicalPoint childPos =
> +      cb.Origin(wm).ConvertTo(childWM, wm, containerSize -
> +                                           childSize.PhysicalSize() -
> +                                           margin.Size(childWM).
> +                                             GetPhysicalSize(childWM));

We should avoid wrapping after a "." IMO, unless it's pretty-necessary for clarity / line-length.  And it's not really necessary here.

So: could you wrap after "wm", to make this look like:

    LogicalPoint childPos =
      cb.Origin(wm).ConvertTo(childWM, wm,
                              containerSize - childSize.PhysicalSize() -
                              margin.Size(childWM).GetPhysicalSize(childWM));

::: layout/generic/nsHTMLReflowState.cpp
@@ +1419,5 @@
>      cbOffset = containingBlock->GetOffsetTo(cbrs->frame);
>    }
> +  nsSize cbrsSize = cbrs->ComputedPhysicalSize() +
> +                    cbrs->ComputedLogicalBorderPadding().Size(cbwm).
> +                      GetPhysicalSize(cbwm);

As above, I'd rather not wrap after a "." unless we really need to.

This can just be:
  nsSize cbrsSize =
    cbrs->ComputedPhysicalSize() +
    cbrs->ComputedLogicalBorderPadding().Size(cbwm).GetPhysicalSize(cbwm);
which is IMO much more readable.

::: layout/generic/nsHTMLReflowState.h
@@ +860,5 @@
>      // use for converting between the logical and physical origins of
>      // the frame. This accounts for the fact that logical origins in RTL
>      // coordinate systems are at the top right of the frame instead of
>      // the top left.
> +    nsSize frameSize = aFrame->GetRect().Size();

No need for GetRect. Just use aFrame->GetSize()

::: layout/generic/nsLineBox.h
@@ +479,2 @@
>        nsPoint physicalDelta = mozilla::LogicalPoint(mWritingMode, 0, aDBCoord).
> +        GetPhysicalPoint(mWritingMode, nullContainerSize);

Wrap before "mozilla::LogicalPoint(" here, so that GetPhysicalPoint isn't de-indented so far to the left of the thing it's being called on.

i.e.: I think this should be:
      nsPoint physicalDelta =
        mozilla::LogicalPoint(mWritingMode, 0, aDBCoord).
          GetPhysicalPoint(mWritingMode, nullContainerSize);

::: layout/generic/nsLineLayout.cpp
@@ +2826,5 @@
>                                                    nscoord aDeltaISize)
>  {
>    PerFrameData* pfd = aPFD->mNextAnnotation;
>    while (pfd) {
> +    nsSize containerSize = pfd->mFrame->GetParent()->GetRect().Size();

Replace "GetRect().Size()" with just "GetSize()".

@@ +3014,5 @@
>        computeState.mFirstParticipant->mJustificationAssignment.mGapsAtStart = 1;
>        computeState.mLastParticipant->mJustificationAssignment.mGapsAtEnd = 1;
>      }
>      nsIFrame* parentFrame = annotation->mFrame->GetParent();
> +    nsSize containerSize = parentFrame->GetRect().Size();

Same here -- "GetRect().Size()" should just be GetSize()

::: layout/generic/nsLineLayout.h
@@ +547,5 @@
>    // The container width to use when converting between logical and
>    // physical coordinates for frames in this span. For the root span
>    // this is the width of the block cached in mContainerSize.width; for
>    // child spans it's the width of the root span
> +  nsSize ContainerSizeForSpan(PerSpanData* aPSD) {

The documentation above this method needs updating. Specifically, it needs:
 - s/mContainerSize.width/mContainerSize/
 - 3 instances of s/width/size/ in prose.

@@ +552,4 @@
>      return (aPSD == mRootSpan)
> +      ? ContainerSize()
> +      : aPSD->mFrame->mBounds.Size(mRootSpan->mWritingMode).
> +        GetPhysicalSize(mRootSpan->mWritingMode);

Two things:
 - s/ContainerSize()/mContainerSize/ to better-match this function's documentation (which mentions the member-var by name), and since there's no reason we need to use the accessor here.

 - Might be worth refactoring this to no longer be a ternary expression, since it's a pretty complex expression (4 lines), for a single ternary statement.  But that doesn't have to happen here.

::: layout/generic/nsRubyFrame.cpp
@@ +253,5 @@
>    }
>  
>    nscoord segmentISize = baseMetrics.ISize(lineWM);
> +  const nsSize dummyContainerSize;
> +  LogicalRect baseRect = aBaseContainer->GetLogicalRect(lineWM, dummyContainerSize);

This line's too long -- wrap after the equals or after the comma.

@@ +318,5 @@
>        } else {
>          MOZ_ASSERT_UNREACHABLE("???");
>        }
>      }
>      // Container width is set to zero here. We will fix it in

This comment (in nsRubyFrame::ReflowSegment, before its call to FinishReflowChild) needs updating to indicate that we're using a dummy container-size (instead of a zero container width).

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +819,5 @@
>      rowFrame = GetRowFrameAt(aPresContext, rowIndex);
>      if (rowFrame) {
>        // translate the coordinates to be relative to us and in our writing mode
>        nsIFrame* frame = rowFrame;
> +      LogicalRect frameRect(wm, frame->GetRect(), aReflowState.ComputedPhysicalSize());

ComputedPhysicalSize() seems wrong here (and ComputedWidth() was also wrong in the old code that you're replacing here), because:
 (1) it's the content-box size, but we want the border-box size
 (2) it might be unconstrained in one dimension or the other.

I think you really want to use aReflowState.ComputedSizeAsContainerIfConstrained() here.

::: layout/tables/nsTableFrame.cpp
@@ +1878,5 @@
>  
>      ReflowTable(aDesiredSize, aReflowState, availBSize,
>                  lastChildReflowed, aStatus);
>      // If ComputedWidth is unconstrained, we may need to fix child positions
> +    // later (in vertical-rl mode) due to use of (0, 0) as a fake containerSize

[This comment ends with "...during ReflowChildren." after the quote above. I'm quoting it explicitly since I don't trust Splinter to correctly quote blocks with adjacent changed/non-changed lines.]

I don't think this comment is quite correct.  ReflowChildren doesn't actually use a 0,0 fake containerSize -- at least, not directly. It calls ComputedSizeAsContainerIfConstrained() to establish its containerSize variable, and that's more complicated. (and probably only 0 in at most one dimension)

(It might use a fake 0,0 container-size in some of its helper methods like DistributeBSizeToRows, but I'm not clear if that's what this comment is talking about.)

**In any case** -- this comment is really describing some width-specific code, so it's fine for it to stay talking just about widths.  So -- we should perhaps just roll this comment back to how it was before this patch, and just replace "containerWidth" with "containerSize.width" in the old text?

@@ +3403,5 @@
>      if (!rgFrame->HasStyleBSize()) {
>        nsTableRowFrame* rowFrame = rgFrame->GetFirstRow();
>        while (rowFrame) {
> +        // We don't know the final width of the rowGroupFrame yet, so use 0,0
> +        // as a "fake" containerSize here; we'll adjust the row positions at

s/"fake"/"dummy"/ for consistency here. (since the variable is called dummyContainerSize)

@@ +3552,5 @@
>        for (nsTableRowFrame* rowFrame = rgFrame->GetFirstRow();
>             rowFrame; rowFrame = rowFrame->GetNextRow()) {
>          nscoord cellSpacingB = GetRowSpacing(rowFrame->GetRowIndex());
> +        // We don't know the final width of the rowGroupFrame yet, so use 0,0
> +        // as a "fake" containerSize here; we'll adjust the row positions at

s/"fake"/"dummy"/ as above.

@@ +3638,5 @@
>  
>        // For vertical-rl mode, we needed to position the rows relative to the
>        // right-hand (block-start) side of the group; but we couldn't do that
>        // above, as we didn't know the rowGroupFrame's final block size yet.
> +      // So we used a containerSize of 0,0 earlier, placing the rows to the

s/containerSize/dummyContainerSize/

@@ +3754,5 @@
>    RowGroupArray orderedRowGroups;
>    OrderRowGroups(orderedRowGroups);
>    nsTableRowFrame* firstRow = nullptr;
>    // XXX not sure if this should be the width of the containing block instead.
> +  nsSize containerSize = mRect.Size();

The XXX comment right before this line ("not sure if this should be the width of the containing block") needs s/width/size/

::: layout/tables/nsTableOuterFrame.cpp
@@ +962,2 @@
>  
>    // Compute the desiredSize so that we can use its Width() as containerWidth

This comment in nsTableOuterFrame::Reflow needs fixup -- perhaps:
 s/use its Width() as containerWidth/use it as the containerSize/
Attachment #8633434 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #35)

> ::: layout/generic/nsFlexContainerFrame.cpp
> @@ +3827,5 @@
> > +  nsSize containerSize;
> > +  containerSize.width = aAxisTracker.IsMainAxisHorizontal() ?
> > +                          aContentBoxMainSize : contentBoxCrossSize;
> > +  containerSize.width +=
> > +    aReflowState.ComputedPhysicalBorderPadding().LeftRight();
> 
> We need a line like this for containerSize.height + [...].TopBottom, too (so
> that containerSize is truly the border-box size of the flex container, in
> both dimensions.)

That fix is in part 2g. I was aiming to minimize actual behavior changes in part 1, so that it'd basically be doing s/containerWidth/containerSize.width/ plus the most essential fallout from that, and implement all the real changes in part 2. 

> ::: layout/generic/nsFloatManager.cpp
> @@ +244,5 @@
> >    // convert back from LineLeft/Right to IStart
> >    nscoord inlineStart = aWM.IsVertical() || aWM.IsBidiLTR()
> > +                        ? lineLeft - mLineLeft
> > +                        : mLineLeft - lineRight +
> > +                          LogicalSize(aWM, aContainerSize).Width(aWM);
> 
> Why the LogicalSize .Width dance here? Can't we just replace this whole last
> line with "aContainerSize.width"?

We'll be wanting to switch this to ISize() in part 2, but you're right, it's premature at this stage.
Attachment #8633434 - Attachment is obsolete: true
Updated following change in part 1.
Attachment #8634005 - Flags: review?(dholbert)
Attachment #8633441 - Attachment is obsolete: true
Attachment #8633441 - Flags: review?(dholbert)
Comment on attachment 8633435 [details] [diff] [review]
part 2 - Respect the container height when converting vertical-RTL inline-direction coordinates

r=me, just one optional nit:

>+++ b/layout/generic/WritingModes.h
>   nscoord LineLeft(WritingMode aWritingMode,
>                    const nsSize& aContainerSize) const
>   {
>     CHECK_WRITING_MODE(aWritingMode);
>     if (aWritingMode.IsVertical()) {
>-      return IStart();
>+      return aWritingMode.IsBidiLTR() ? IStart()
>+                                      : aContainerSize.height - IEnd();
>     } else {
>       return aWritingMode.IsBidiLTR() ? IStart()
>                                       : aContainerSize.width - IEnd();
>     }

The logic seems a bit duplicated/redundant here, with the the two separate IsBidiLTR checks which do the same thing on success.

Maybe we should pull the IsBidiLTR() check out to be the first thing we check? Something like the following (haven't actually tested):

    if (aWritingMode.IsBidiLTR()) {
      nscoord containerISize =
        aWritingMode.IsVertical() ? aContainerSize.height : aContainerSize.width;
      return containerISize - IEnd();
    }
    return IStart();

Same goes for the LineRight() impl.

Anyway, r=me regardless.
Attachment #8633435 - Flags: review?(dholbert) → review+
Comment on attachment 8633436 [details] [diff] [review]
part 2a - Remove hack for rtl-in-vertical-mode from ReflowAbsoluteFrame

r=me
Attachment #8633436 - Flags: review?(dholbert) → review+
Comment on attachment 8633437 [details] [diff] [review]
part 2b - Mark relative-overconstrained tests that now pass in vertical mode with rtl

r=me
Attachment #8633437 - Flags: review?(dholbert) → review+
Comment on attachment 8633438 [details] [diff] [review]
part 2c - Mark vertical border-collapse bevel tests that now pass

r=me
Attachment #8633438 - Flags: review?(dholbert) → review+
Comment on attachment 8633439 [details] [diff] [review]
part 2d - Remove partial rtl-in-vertical support from nsBidiPresUtils now that logical-coordinate classes handle it better

So this patch moves |margin| without redefining it, which is fine...
>+++ b/layout/base/nsBidiPresUtils.cpp
>-  LogicalMargin margin = frameMargin.ConvertTo(aContainerWM, frameWM);
[...]
>+  LogicalMargin margin = frameMargin.ConvertTo(aContainerWM, frameWM);

...but then further down, it removes a usage of |margin|, and I'm not clear on why:
>-  return icoord + margin.IStartEnd(aContainerWM);
[...]
>+  return icoord +
>+         frameMargin.ConvertTo(aContainerWM, frameWM).IStartEnd(aContainerWM);

Isn't "frameMargin.ConvertTo(aContainerWM, frameWM)" in this new, more-verbose return statement still just equivalent to "margin"?

(If so: can't we leave this return statement untouched? I don't think |margin| gets modified after it's declared...)
(In reply to Daniel Holbert [:dholbert] from comment #44)
> So this patch moves |margin| without redefining it, which is fine...

(Sorry, "redefining" is vague here. I meant to say "...without changing its value/meaning".)
Comment on attachment 8633442 [details] [diff] [review]
part 2f - Mark vertical-mode float-in-rtl reftests that are now passing

r=me on 2f. (Hooray for making piles of tests go from failing to passing!)
Attachment #8633442 - Flags: review?(dholbert) → review+
Comment on attachment 8633443 [details] [diff] [review]
part 2g - Compute both dimensions of containerSize in nsFlexContainerFrame::DoLayout

>+++ b/layout/generic/nsFlexContainerFrame.cpp
>@@ -3819,21 +3819,25 @@ nsFlexContainerFrame::DoFlexLayout(nsPre
>+  // Set up container size on the assumption that the main axis is inline...
>+  LogicalSize logSize(flexWM, aContentBoxMainSize, contentBoxCrossSize);
>+  // ...then swap its fields if this is not the case
>+  if (flexWM.IsVertical() == aAxisTracker.IsMainAxisHorizontal()) {
>+    Swap(logSize.ISize(flexWM), logSize.BSize(flexWM));
>+  }

(Tangent: asking "is the main axis inline" in a flex container is equivalent to asking "do we have flex-direction:row", which you can check with aAxisTracker.IsRowOriented().  The "IsMainAxisHorizontal" function you're using here is deprecated and is going away soon.)

Two changes I'd suggest here:
(1) You don't need the inline-axis check / swapping here -- there's a utility function you can use:
  LogicalSize logSize =
    aAxisTracker.LogicalSizeFromFlexRelativeSizes(aContentBoxMainSize,
                                                  contentBoxCrossSize);

 (2) Given (1), the initial "assumption that the main axis is inline..." comment can be simplified a bit (& clarified while we're at it), to something like:
  // Determine flex container's border-box size (used in positioning children):

r=me with that.
Attachment #8633443 - Flags: review?(dholbert) → review+
Comment on attachment 8633444 [details] [diff] [review]
part 2h - Mark flexbox writing-mode tests that are now passing

r=me on 2h   \o/
Attachment #8633444 - Flags: review?(dholbert) → review+
Comment on attachment 8634005 [details] [diff] [review]
part 2e - Remove hack for float positioning in vertical mode with di

r=me on 2e
Attachment #8634005 - Flags: review?(dholbert) → review+
Comment on attachment 8633439 [details] [diff] [review]
part 2d - Remove partial rtl-in-vertical support from nsBidiPresUtils now that logical-coordinate classes handle it better

r=me on 2d, modulo my question about 'margin' & why-are-we-modifying-the-return-statement in comment 44.

(I don't 100% understand what the code that you're touching here is doing, but it does look like the transformation to logical units (& relying on that for logical RTL handling) is correct.)
Attachment #8633439 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #44)

> Isn't "frameMargin.ConvertTo(aContainerWM, frameWM)" in this new,
> more-verbose return statement still just equivalent to "margin"?
> 
> (If so: can't we leave this return statement untouched? I don't think
> |margin| gets modified after it's declared...)

Indeed. I guess it must have gone through some evolutionary process that ended up back where it started (in a more verbose form), but I don't recall exactly how that went! Anyhow, I've cleaned it up, thanks.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/28673cc5e68b48d6a397b027a6ef5321703dea4a
changeset:  28673cc5e68b48d6a397b027a6ef5321703dea4a
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Jul 16 10:07:57 2015 +0100
description:
Bug 1131451 part 1 - Replace containerWidth with containerSize in logical-coordinate classes and APIs, frame classes, etc. r=dholbert

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/e6eed9f588de74077f18248d72508418d7061c23
changeset:  e6eed9f588de74077f18248d72508418d7061c23
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Jul 16 10:08:05 2015 +0100
description:
Bug 1131451 part 2 - Respect the container height when converting vertical-RTL inline-direction coordinates. r=dholbert
* * *
Bug 1131451 part 2a - Remove hack for rtl-in-vertical-mode from ReflowAbsoluteFrame. r=dholbert
* * *
Bug 1131451 part 2b - Mark relative-overconstrained tests that now pass in vertical mode with rtl. r=dholbert
* * *
Bug 1131451 part 2c - Mark vertical border-collapse bevel tests that now pass. r=dholbert
* * *
Bug 1131451 part 2d - Remove partial rtl-in-vertical support from nsBidiPresUtils now that logical-coordinate classes handle it better. r=dholbert
* * *
Bug 1131451 part 2e - Remove hack for float positioning in vertical mode with dir=rtl. r=dholbert
* * *
Bug 1131451 part 2f - Mark vertical-mode float-in-rtl reftests that are now passing. r=dholbert
* * *
Bug 1131451 part 2g - Compute both dimensions of containerSize in nsFlexContainerFrame::DoLayout. r=dholbert
* * *
Bug 1131451 part 2h - Mark flexbox writing-mode tests that are now passing. r=dholbert
Here's one I missed; it shouldn't be needed, and tryserver confirms tests continue to pass with this removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=811a5e0e1e78.
Attachment #8634698 - Flags: review?(dholbert)
Attachment #8634698 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/28673cc5e68b
https://hg.mozilla.org/mozilla-central/rev/e6eed9f588de
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/54c94c8b16c93ec78c2bb6362ccbd455b165105f
changeset:  54c94c8b16c93ec78c2bb6362ccbd455b165105f
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Jul 16 21:04:06 2015 +0100
description:
Bug 1131451 followup - Remove one more bidi-in-vertical-mode hack that is no longer required. r=dholbert
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: