convert nsTableOuterFrame to work with logical coordinates

RESOLVED FIXED in Firefox 42

Status

()

Core
Layout: Tables
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla42
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8627872 [details] [diff] [review]
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.)
Attachment #8627872 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
Created attachment 8627874 [details] [diff] [review]
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.)
Attachment #8627874 - Flags: review?(dholbert)
(Assignee)

Comment 6

3 years ago
Created attachment 8627875 [details] [diff] [review]
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.
Attachment #8627875 - Flags: review?(cam)
(Assignee)

Comment 7

3 years ago
Created attachment 8627882 [details] [diff] [review]
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.)
Attachment #8627882 - Flags: review?(dholbert)
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.
Attachment #8627872 - Flags: review?(cam) → review+
Attachment #8627875 - Flags: review?(cam) → review+
(Assignee)

Comment 9

3 years ago
(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.)
(Assignee)

Comment 10

3 years ago
Created attachment 8628774 [details] [diff] [review]
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.
Attachment #8628774 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8627882 - Attachment is obsolete: true
Attachment #8627882 - Flags: review?(dholbert)
Attachment #8628774 - Flags: review?(dholbert) → review+
(Assignee)

Comment 12

3 years ago
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.
Attachment #8628969 - Flags: review?(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 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.
Attachment #8627874 - Flags: review?(dholbert) → review+
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.
Attachment #8628969 - Flags: review?(dholbert) → review-
(Assignee)

Comment 16

3 years ago
Created attachment 8629246 [details] [diff] [review]
patch 4 - Reftests for logical and physical caption-side placement

Let's include the actual files this time. (oops)
Attachment #8629246 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8628969 - Attachment is obsolete: true
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.
Attachment #8629246 - Flags: review?(dholbert) → review+
(Assignee)

Comment 18

3 years ago
(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 22

3 years ago
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
(Assignee)

Comment 23

3 years ago
(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.
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.