Part of support for vertical text

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.

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

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

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

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

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

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

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.

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

Changed to pass LogicalPoints by const ref per comment 9 Transferring r=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. :(

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.

Again, adopting the suggestion from comment 12

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

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.

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?

As I said above, this is no longer needed.

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

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

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

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

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 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 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?

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

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

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

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

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

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

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

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?

Yes, let's do that