1177076 - convert nsTableOuterFrame to work with logical coordinates

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jonathan Kew [:jfkthame]]

There's still a lot of physical, horizontal-only code here; converting this looks like it'll be necessary to handle captions properly, for example.

Comment 1

[Jonathan Kew [:jfkthame]]

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

Comment 2

[Jonathan Kew [:jfkthame]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

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

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Add logical values for the caption-side property (though not yet handled by layout code)

As I needed the new logical values for caption-side in order to work on nsTableOuterFrame, I put together a patch to add them on the CSS side. (The next patch will update the relevant layout code to actually handle the new values.)

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Convert nsTableOuterFrame to work with logical coordinates

This makes nsTableOuterFrame aware of writing-mode, so that captions work properly in vertical mode; even captions that are orthogonal to the table's writing mode work reasonably well. (Tests to follow.)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Change the initial value of 'caption-side' from 'top' (physical) to 'block-start' (logical)

Now that the table code handles the logical caption-side values, we should change the initial value to block-start.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: patch 2.1 - Annotate vertical-table-specified-width-2.html because it will assert in FinishReflowChild() due to unconstrained containerWidth

Currently patch 2 here will result in one unexpected assertion (see XXX comment added in nsTableOuterFrame::Reflow); nevertheless, the testcase renders as expected. I suggest we annotate that test as expecting the assertion for now, and file a followup to investigate it further. (It's got something to do with the reflowState's computed margins.)

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8627872 [details] [diff] [review]
patch 1 - Add logical values for the caption-side property (though not yet handled by layout code)

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

Will you mail www-style requesting to add the block-{start,end}-outside values?

::: layout/generic/WritingModes.h
@@ +360,5 @@
> +   * Returns the logical side corresponding to the specified physical side,
> +   * given the current writing mode.
> +   * (This is the inverse of the PhysicalSide() method above.)
> +   */
> +  LogicalSide LogicalSideForPhysicalSide(mozilla::css::Side aSide) const

It's a shame this can't be named LogicalSide() (presumably due to it conflicting with the name of the type), to parallel PhysicalSide().

@@ +2032,5 @@
> +    case NS_STYLE_CAPTION_SIDE_TOP:
> +    case NS_STYLE_CAPTION_SIDE_RIGHT:
> +    case NS_STYLE_CAPTION_SIDE_BOTTOM:
> +    case NS_STYLE_CAPTION_SIDE_LEFT:
> +    {

Nit: Put "{" on previous line.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #8)
> Will you mail www-style requesting to add the block-{start,end}-outside
> values?

Will do.

> > +  LogicalSide LogicalSideForPhysicalSide(mozilla::css::Side aSide) const
> 
> It's a shame this can't be named LogicalSide() (presumably due to it
> conflicting with the name of the type), to parallel PhysicalSide().

Yeah. (I think we could do it that way, actually, by explicitly writing 'enum LogicalSide' to access the type in cases where it's hidden by the method, but it seemed to me that it'd be clearer to use the long name and avoid confusion.)

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: patch 2.1 - Annotate vertical-table-specified-width-2.html because it will assert in FinishReflowChild() due to unconstrained containerWidth

The assertion here is related to the problem with nsHTMLReflowState::CalculateBlockSideMargins noted in bug 1179741.

Comment 11

[Jonathan Kew [:jfkthame]]

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

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Reftests for logical and physical caption-side placement

Here's a collection of basic reftests to check that caption-side puts things in the right place.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8627874 [details] [diff] [review]
patch 2 - Convert nsTableOuterFrame to work with logical coordinates

Review feedback for patch 2, "wave 1" (of 2):

>+++ b/layout/tables/nsTableOuterFrame.cpp
>@@ -151,17 +151,17 @@ nsTableOuterFrame::InsertFrames(ChildLis
> void
> nsTableOuterFrame::RemoveFrame(ChildListID     aListID,
>                                nsIFrame*       aOldFrame)
> {
[...]
> 
>-  if (HasSideCaption()) {
>+  if (HasSideCaption(GetWritingMode())) {
>     // The old caption width had an effect on the inner table width so
>     // we're going to need to reflow it. Mark it dirty

s/width/isize/ (2x) in this comment

>@@ -260,31 +260,30 @@ nsTableOuterFrame::GetChildMargin(nsPres
>                                   nscoord                  aAvailISize,
>                                   LogicalMargin&           aMargin)
> {
>   // construct a reflow state to compute margin and padding. Auto margins
>   // will not be computed at this time.
> 
>   // create and init the child reflow state
>   // XXX We really shouldn't construct a reflow state to do this.
>-  WritingMode wm = aChildFrame->GetWritingMode();
>+  WritingMode wm = aOuterRS.GetWritingMode();
>   LogicalSize availSize(wm, aAvailISize, aOuterRS.AvailableSize(wm).BSize(wm));
>   nsHTMLReflowState childRS(aPresContext, aOuterRS, aChildFrame, availSize,
>                             nullptr, nsHTMLReflowState::CALLER_WILL_INIT);

(I think this change is assuming that aChildFrame is *not* a caption frame here, right? If it were a caption, we couldn't just copy the outer WM here, because it might be different.

Happily, I think this assumption is valid -- doesn't look like caption frames make it here, from looking at how this method is called.  Maybe add an assertion to verify !aChildFrame->IsTableCaption() here, though, to be sure?)

> static nscoord
> ChildShrinkWrapISize(nsRenderingContext *aRenderingContext,
[...]
>   AutoMaybeDisableFontInflation an(aChildFrame);
>   LogicalSize size =
>     aChildFrame->ComputeSize(aRenderingContext,
[...]
>+                  (offsets.ComputedLogicalBorderPadding().Size(childWM) -
>+                    offsets.ComputedLogicalPadding().Size(childWM)).
>+                    ConvertTo(aWM, childWM),
>+                  offsets.ComputedLogicalPadding().Size(childWM).
>+                    ConvertTo(aWM, childWM),
>                   nsIFrame::ComputeSizeFlags::eShrinkWrap);

Hmm, this ends up pretty unwieldy, with 2- or 3-line function calls with varied indentation being passed in as individual args to ComputeSize (largely because of all of the ConvertTo calls that we need now). This might be cleaner if we pull out some of these into local variables -- what do you think? (Either calling ConvertTo() up-front when populating the local vars, or calling it on them as we pass them into ComputeSize; whatever makes sense)

I'll leave it up to you whether you want to change anything here.

>+nsTableOuterFrame::SetDesiredSize(uint8_t              aCaptionSide,
>+                                  const LogicalSize&   aInnerSize,
>+                                  const LogicalSize&   aCaptionSize,
>+                                  const LogicalMargin& aInnerMargin,
>+                                  const LogicalMargin& aCaptionMargin,
>+                                  nscoord&             aISize,
>+                                  nscoord&             aBSize,
>+                                  WritingMode          aWM)
> {
[...]
>+  // compute the overall block-size
>+  switch (aCaptionSide) {
[...]
>+    default:
>+      aBSize = aInnerSize.BSize(aWM) + aInnerMargin.BStartEnd(aWM);
>+      break;
>+  }

I think this 'default' case is really only for NO_SIDE, right? (It's the only case that ignores the caption).

Please add a comment here to this effect. (or better, a NS_ASSERTION, if we can.)

>+nsTableOuterFrame::GetCaptionOrigin(uint32_t             aCaptionSide,
[...]
>+    case NS_STYLE_CAPTION_SIDE_BEND:
>+    case NS_STYLE_CAPTION_SIDE_BEND_OUTSIDE:
>+      // FIXME: Position relative to right edge for RTL.  (Based on table
>+      // direction or table parent direction?)
>+      aOrigin.I(aWM) = aCaptionMargin.IStart(aWM);

This FIXME (currently preserved from the old code) is probably no longer applicable, now that we're using .I() instead of .x here.

Either drop the comment (since it's now misleading), or (if you're unsure) add a caveat that it might no longer be applicable now that we're using logical directions.

(This comment is repeated at the bottom of this switch statement, too -- both copies need removing/fixing.)

>+      if (aCaptionSide == NS_STYLE_CAPTION_SIDE_BEND) {
>+        // We placed the caption using only the table's width as available
>+        // width, and we should position it this way as well.
>+        aOrigin.I(aWM) += aInnerMargin.IStart(aWM);

Comment needs 2x s/width/iSize/

(This comment is repeated at the bottom of this switch statement, too -- both copies need fixing.)

>+    case NS_STYLE_CAPTION_SIDE_IEND:
>+      aOrigin.I(aWM) = aInnerMargin.IStart(aWM) + aInnerSize.ISize(aWM) +
>+                         aCaptionMargin.IStart(aWM);

Nit: the indentation seems odd here -- I think our standard Mozilla style would have the last line indented only 2 spaces past the very start of the previous line.  But I can also see it being deindented to be directly below "aInnerMargin", too, if you'd prefer. (I don't think it makes sense to have it indented *past* aInnerMargin like it is now, though. Usually that only happens we're entering a new level of nesting, not when we split arithmetic across several lines.)

>       switch (GetCaptionVerticalAlign()) {
>         case NS_STYLE_VERTICAL_ALIGN_MIDDLE:
>-          aOrigin.y = std::max(0, aInnerMargin.top + ((aInnerSize.height - aCaptionSize.height) / 2));
>+          aOrigin.B(aWM) = std::max(0, aInnerMargin.BStart(aWM) +
>+                                       ((aInnerSize.BSize(aWM) -
>+                                         aCaptionSize.BSize(aWM)) / 2));
>           break;
>         case NS_STYLE_VERTICAL_ALIGN_BOTTOM:
>-          aOrigin.y = std::max(0, aInnerMargin.top + aInnerSize.height - aCaptionSize.height);
>+          aOrigin.B(aWM) = std::max(0, aInnerMargin.BStart(aWM) +
>+                                         aInnerSize.BSize(aWM) -
>+                                         aCaptionSize.BSize(aWM));

The last 2 lines here need 2 spaces of de-indentation.

>+    case NS_STYLE_CAPTION_SIDE_BEND_OUTSIDE:
>+    case NS_STYLE_CAPTION_SIDE_BEND:
>+      aOrigin.B(aWM) = aInnerMargin.BStart(aWM) + aInnerSize.BSize(aWM) +
>+                         aCaptionMargin.BStart(aWM);

Last line here needs de-indentation, similar to comment above about the aOrigin.I(aWM) = ... expression.

>+nsTableOuterFrame::GetInnerOrigin(uint32_t             aCaptionSide,
[...]
>+  // inline-dir computation
>   switch (aCaptionSide) {
>-  case NS_STYLE_CAPTION_SIDE_LEFT: {
>-    if (aInnerMargin.left < minCapWidth) {
>+  case NS_STYLE_CAPTION_SIDE_ISTART: {
[...]
>   } break;
>   }

If you like: the body of this switch statement in GetInnerOrigin really needs 2 more spaces of indentation. (to be indented past the "switch" line itself)

(I noticed you fix up this same sort of thing elsewhere in this patch, but not this particular spot.  If you want to fix this one up as well, great; if not, we can also do it separately too.)

>+    case NS_STYLE_CAPTION_SIDE_IEND:
>+      aOrigin.B(aWM) = aInnerMargin.BStart(aWM);
>       switch (GetCaptionVerticalAlign()) {
>         case NS_STYLE_VERTICAL_ALIGN_MIDDLE:
>-          aOrigin.y = std::max(aInnerMargin.top, (aCaptionSize.height - aInnerSize.height) / 2);
>+          aOrigin.B(aWM) = std::max(aInnerMargin.BStart(aWM),
>+                                    (aCaptionSize.BSize(aWM) -
>+                                      aInnerSize.BSize(aWM)) / 2);

As above, aInnerSize should be indented 2 spaces less here.

>         case NS_STYLE_VERTICAL_ALIGN_BOTTOM:
>-          aOrigin.y = std::max(aInnerMargin.top, aCaptionSize.height - aInnerSize.height);
>+          aOrigin.B(aWM) = std::max(aInnerMargin.BStart(aWM),
>+                                    aCaptionSize.BSize(aWM) -
>+                                      aInnerSize.BSize(aWM));

and here.

>+    case NS_STYLE_CAPTION_SIDE_BSTART_OUTSIDE:
>+    case NS_STYLE_CAPTION_SIDE_BSTART:
>+      aOrigin.B(aWM) = aInnerMargin.BStart(aWM) + aCaptionSize.BSize(aWM) +
>+                         aCaptionMargin.BStartEnd(aWM);

(and aCaptionMargin should be indented 2-fewer or many-fewer spaces here.) 

> void
> nsTableOuterFrame::OuterDoReflowChild(nsPresContext*             aPresContext,
>                                       nsIFrame*                  aChildFrame,
>                                       const nsHTMLReflowState&   aChildRS,
>                                       nsHTMLReflowMetrics&       aMetrics,
>                                       nsReflowStatus&            aStatus)
> { 
>+  WritingMode wm = aChildRS.GetWritingMode();
>+  LogicalPoint childPt = aChildFrame->GetLogicalPosition(wm, 0);
[...]
>   ReflowChild(aChildFrame, aPresContext, aMetrics, aChildRS,
>-              childPt.x, childPt.y, flags, aStatus);
>+              wm, childPt, 0, flags, aStatus);

The "0" in both of these expressions is for containerWidth, I think; right now it's a bit magic.

Assuming it's actually fine to use 0 here, I'd prefer something like...
  // Using 0 containerWidth for rtl computations for the moment because $REASON.
  const nscoord zeroCWidth = 0;

  LogicalPoint childPt = aChildFrame->GetLogicalPosition(wm, zeroCWidth);
  [...]
  ReflowChild(aChildFrame, aPresContext, aMetrics, aChildRS,
              wm, childPt, zeroCWidth, flags, aStatus);

...to make these "0" args less magic/mysterious.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8627874 [details] [diff] [review]
patch 2 - Convert nsTableOuterFrame to work with logical coordinates

Review feedback for patch 2, "wave 2" (of 2) -- just one comment, actually:
>@@ -817,148 +852,160 @@ nsTableOuterFrame::Reflow(nsPresContext*
>+  LogicalSize captionSize(wm);
>+  LogicalMargin captionMargin(captionWM);
>   if (mCaptionFrames.NotEmpty()) {
[...]
>+          captionBSize = captionSize.BSize(wm) + captionMargin.BStartEnd(wm);
>           break;

Won't passing 'wm' to captionMargin here make us assert? (since it was initialized with captionWM)

(Don't we need to convert it into writing-mode "wm" before calling BStartEnd on it here? You already do this later down, under the comment:
   // Ensure we have all margins in the table's writing mode now.
but maybe we need to do it up here, though?)

r=me with this & "wave 1" notes addressed as you see fit.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8628969 [details] [diff] [review]
patch 4 - Reftests for logical and physical caption-side placement

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Created attachment 8628969 [details] [diff] [review]
> patch 4 - Reftests for logical and physical caption-side placement
> 
> Here's a collection of basic reftests to check that caption-side puts things
> in the right place.

This patch only has reftest.list changes -- I think you forgot to 'hg add' the test files themselves.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Reftests for logical and physical caption-side placement

Let's include the actual files this time. (oops)

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8629246 [details] [diff] [review]
patch 4 - Reftests for logical and physical caption-side placement

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

::: layout/reftests/writing-mode/tables/table-caption-top-1-ref.html
@@ +10,5 @@
> +  margin-bottom: 10px;
> +}
> +.h {
> +  -webkit-writing-mode: horizontal-tb;
> +  writing-mode: horizontal-tb;

I'm not sure it's useful to have these -webkit prefixed styles here, given that
 (1) Chrome doesn't render the testcase correctly even with these styles -- e.g. it puts some "top" captions end up on the left & right sides.
 (2) Chrome can't be expected to render the reference cases correctly, due to usage of "-moz-min-content" and (to a lesser degree) the "inline-size" logical property there.

Maybe it's still handy for comparisons, though. Anyway, r=me regardless.

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 8629246 [details] [diff] [review]
> patch 4 - Reftests for logical and physical caption-side placement
> 
> Review of attachment 8629246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/writing-mode/tables/table-caption-top-1-ref.html
> @@ +10,5 @@
> > +  margin-bottom: 10px;
> > +}
> > +.h {
> > +  -webkit-writing-mode: horizontal-tb;
> > +  writing-mode: horizontal-tb;
> 
> I'm not sure it's useful to have these -webkit prefixed styles here, given
> that
>  (1) Chrome doesn't render the testcase correctly even with these styles --
> e.g. it puts some "top" captions end up on the left & right sides.
>  (2) Chrome can't be expected to render the reference cases correctly, due
> to usage of "-moz-min-content" and (to a lesser degree) the "inline-size"
> logical property there.

I'll remove them, they're really just a hangover from my local experimentation. Chrome's support for caption-side seems to be much more limited at present anyhow.

Comment 19

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7938cd20bee3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8864f9b4bb29
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ac80fce0e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf7a6bcd76f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4febbf3444f9

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d42e57177cb

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7938cd20bee3
https://hg.mozilla.org/mozilla-central/rev/8864f9b4bb29
https://hg.mozilla.org/mozilla-central/rev/d9ac80fce0e2
https://hg.mozilla.org/mozilla-central/rev/4bf7a6bcd76f
https://hg.mozilla.org/mozilla-central/rev/4febbf3444f9
https://hg.mozilla.org/mozilla-central/rev/9d42e57177cb

Comment 22

[Gérard Talbot]

Interactive caption-side demo for all writing-modes
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/CaptionSide-writing-modes-dhtml.html

Note that [top | middle | bottom] 'vertical-align' values can be dynamically set for 'caption-side: left' and 'caption-side: right'... but, in all fairness, this is a CSS2.1 spec violation.

(In reply to Cameron McCormack (:heycam) from comment #8)

> Will you mail www-style requesting to add the block-{start,end}-outside
> values?

Where is the documentation on top-outside and bottom-outside and their logical correspondent block-{start,end}-outside values? I have searched and did not find any.

Gérard

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to Gérard Talbot from comment #22)
> Where is the documentation on top-outside and bottom-outside and their
> logical correspondent block-{start,end}-outside values? I have searched and
> did not find any.

Yes, they seem to be missing from https://developer.mozilla.org/en-US/docs/Web/CSS/caption-side. I don't have time to try and update that page just now, but in brief: the -outside values allow the caption to extend wider than the body of the table (if the table's container permits).

Marking dev-doc-needed for the missing *-outside values.

Comment 24

[Jean-Yves Perrier [:teoli]]

Sebastian (:sebo) updated the bug wrt to these two missing values back in July.

Setting dev-doc-complete, but this page will need a technical review (once updated with the latest change in top/bottom meaning wrt to writing-mode).

https://developer.mozilla.org/en-US/docs/Web/CSS/caption-side