1174711 - Convert nsTableFrame to work with logical coordinates

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Comment 1

[Jonathan Kew [:jfkthame]]

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

Comment 2

[Jonathan Kew [:jfkthame]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Rename nsIPercentHeightObserver to nsIPercentBSizeObserver, and update related frame methods to match

A preliminary step, before attempting the main nsTableFrame conversion...

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Convert nsTableFrame to work with logical coordinates. try: -b do -p linux64 -u all

This converts enough of nsTableFrame that simple examples seem to work pretty well. Some major parts NOT yet addressed: pagination (see also nsTableRowGroupFrame) and headers/footers, and rendering of collapsed borders in vertical mode. These will need to be handled by additional patches here or in followup bugs.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Rename a couple more frame-state bits from physical to logical. try: -b do -p linux64 -u all

Renaming some more of the frame-state bits that are now logical.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Rename mSpecialHeightReflow to mSpecialBSizeReflow, and update comments to match

And let's rename the mSpecialHeightReflow flag to match its logical-coordinate meaning, and update comments accordingly. (Purely a sed patch.)

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Rename nsIPercentHeightObserver to nsIPercentBSizeObserver, and update related frame methods to match

Minor rebase on top of updates in bug 1174700.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8624787 [details] [diff] [review]
patch 1 - Rename nsIPercentHeightObserver to nsIPercentBSizeObserver, and update related frame methods to match

r=me on patch 1 with the following:

>+++ b/layout/generic/nsHTMLReflowState.h
>   // a frame (e.g. nsTableCellFrame) which may need to generate a special 
>   // reflow for percent height calculations 
>-  nsIPercentHeightObserver* mPercentHeightObserver;
>+  nsIPercentBSizeObserver* mPercentBSizeObserver;

Comment needs s/height/bsize/ (in "percent height calculations")

>+++ b/layout/tables/nsTableCellFrame.cpp
> // The cell needs to observe its block and things inside its block but nothing below that
> bool
> nsTableCellFrame::NeedsToObserve(const nsHTMLReflowState& aReflowState)
> {
[...]
>   // We need the observer to be propagated to all children of the cell
>   // (i.e., children of the child block) in quirks mode, but only to

I not sure if this chunk is correct anymore (at the very end of this function), in the brave new world of orthogonal flows.  Specifically: if children of the cell's block have a WM that's orthogonal to the cell's WM, it probably doesn't make sense for the cell to observe their percent BSizes. (though maybe we need to observe their percent ISizes?)

Please add an XXX comment here, at least, to point out that this case may need tweaking if children of the child block create an orthogonal flow.

> // Notify the frame and its ancestors (up to the containing table) that a special
> // height reflow will occur. During a special height reflow, a table, row group,
> // row, or cell returns the last size it was reflowed at. However, the table may
> // change the height of row groups, rows, cells in DistributeHeightToRows after.
> // And the row group can change the height of rows, cells in CalculateRowHeights.
> void
>-nsTableFrame::RequestSpecialHeightReflow(const nsHTMLReflowState& aReflowState)
>+nsTableFrame::RequestSpecialBSizeReflow(const nsHTMLReflowState& aReflowState)
> {

This function's documentation (in both nsTableFrame.cpp & .h) needs updating with s/height/bsize/ in "special height reflow", to stay in sync with the function name.  

(I initially thought Patch 4 might cover this, but it doesn't. Feel free to fix this in part 4 if you'd like, if that helps avoid bitrot)

>@@ -917,23 +917,23 @@ nsTableCellFrame::Reflow(nsPresContext* 
>   // Don't be a percent height observer if we're in the middle of
>-  // special-height reflow, in case we get an accidental NotifyPercentHeight()
>+  // special-height reflow, in case we get an accidental NotifyPercentBSize()
>   // call (which we shouldn't honor during special-height reflow)
>   if (!aReflowState.mFlags.mSpecialHeightReflow) {
>-    // mPercentHeightObserver is for children of cells in quirks mode,
>+    // mPercentBSizeObserver is for children of cells in quirks mode,

This comment needs s/height/bsize/, in "percent height observer" on the first line.

(Patch 4 adjusts other parts of this comment, with s/special-height/special-bsize/, but it doesn't fix 'percent height observer'.)

(If you want to avoid bitrotting yourself, feel free to just clean up this "percent height" usage in Patch 4 instead of in this patch.)

>+++ b/layout/tables/nsTableFrame.cpp
> // Return true if aParentReflowState.frame or any of its ancestors within
> // the containing table have non-auto height. (e.g. pct or fixed height)
> bool
>-nsTableFrame::AncestorsHaveStyleHeight(const nsHTMLReflowState& aParentReflowState)
>-{
>+nsTableFrame::AncestorsHaveStyleBSize(const nsHTMLReflowState& aParentReflowState)
>+{

Comment needs s/height/bsize/  (2x)

>@@ -1809,126 +1812,128 @@ nsTableFrame::Reflow(nsPresContext*     
>+    if (aReflowState.ComputedBSize() != NS_UNCONSTRAINEDSIZE ||
>         // Also check mVResize, to handle the first Reflow preceding a
>         // special height Reflow, when we've already had a special height
>         // Reflow (where mComputedHeight would not be
>         // NS_UNCONSTRAINEDSIZE, but without a style change in between).
>-        aReflowState.IsVResize()) {
>+        aReflowState.IsBResize()) {

Comment needs s/mVResize/IsBResize()/ to stay in sync with the code here.

>     // see if an extra reflow will be necessary in pagination mode when there is a specified table height
>-    if (isPaginated && !GetPrevInFlow() && (NS_UNCONSTRAINEDSIZE != aReflowState.AvailableHeight())) {
>-      nscoord tableSpecifiedHeight = CalcBorderBoxHeight(aReflowState);
[...]
>+    if (isPaginated && !GetPrevInFlow() && (NS_UNCONSTRAINEDSIZE != aReflowState.AvailableBSize())) {
>+      nscoord tableSpecifiedBSize = CalcBorderBoxHeight(aReflowState);

Comment needs s/height/bsize/ in "specified table height" to stay in sync with the code here.

>       if (lastChildReflowed && NS_FRAME_IS_NOT_COMPLETE(aStatus)) {
>         // if there is an incomplete child, then set the desired height to include it but not the next one
>         LogicalMargin borderPadding = GetChildAreaOffset(wm, &aReflowState);
>-        aDesiredSize.Height() = borderPadding.BEnd(wm) + GetRowSpacing(GetRowCount()) +
>-                              lastChildReflowed->GetNormalRect().YMost();
>+        aDesiredSize.BSize(wm) =
>+          borderPadding.BEnd(wm) + GetRowSpacing(GetRowCount()) +
>+          lastChildReflowed->GetNormalRect().YMost(); // XXX

I assume this "// XXX" is about the YMost usage in a BSize expression here?

Did you mean to leave this XXX in? (if so, might be worth adding an actual comment hinting at what's wrong. e.g. "YMost should be B-flavored")

>-  aDesiredSize.Width() = aReflowState.ComputedWidth() +
>-                       aReflowState.ComputedPhysicalBorderPadding().LeftRight();
>+  aDesiredSize.ISize(wm) = aReflowState.ComputedISize() +
>+                       aReflowState.ComputedLogicalBorderPadding().IStartEnd(wm);

De-indent last line here, to stay under 80 chars and since the new (and old) indentation amount is arbitrary).

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8624209 [details] [diff] [review]
patch 2 - Convert nsTableFrame to work with logical coordinates. try: -b do -p linux64 -u all

Patch 2 review comments, "wave 1":
==================================

>+++ b/layout/tables/nsTableFrame.h
>   /**
>-   * A copy of nsFrame::ShrinkWidthToFit that calls a different
>+   * A copy of nsFrame::ShrinkISizeToFit that calls a different
>    * GetPrefISize, since tables have two different ones.
>    */
>-  nscoord TableShrinkWidthToFit(nsRenderingContext *aRenderingContext,
>+  nscoord TableShrinkISizeToFit(nsRenderingContext *aRenderingContext,
>                                 nscoord aWidthInCB);

We should revert the s/Width/ISize/ tweak in the comment here -- this comment is talking about a specific function which is yet-to-be renamed, so we shouldn't rename this mention of it.

(I imagine it will be renamed eventually, but I don't think it's part of this current effort. When that rename happens, I expect we'll do a search-and-replace & catch this usage. It currently lives here, FWIW:
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.h?rev=c671c18ebf38#296
)

>+++ b/layout/tables/nsTableRowGroupFrame.h
>-    * @param aYTotalOffset the total amount that the rowgroup is shifted up
>-    * @param aWidth        new width of the rowgroup
>+    * @return              additional shift bstart-wards that should be applied
>+    *                      to subsequent rowgroups due to rows and this
>+    *                      rowgroup being collapsed
>+    * @param aBTotalOffset the total amount that the rowgroup is shifted
>+    *                      bstart-wards
>+    * @param aISize        new isize of the rowgroup
>     */
>   nscoord CollapseRowGroupIfNecessary(nscoord aBTotalOffset,
>-                                      nscoord aISize);
>+                                      nscoord aISize,
>+                                      mozilla::WritingMode aWM);

(Note: This patch-chunk doesn't apply anymore, but that's just because this comment fixup is now happening over in the main patch on bug 1174700 (attachment 8625078 [details] [diff] [review]), which makes sense because that's where the args are being renamed.  So the only thing left for this patch to do here is to add the new 'aWM' param to the function-signature.)

>+++ b/layout/tables/nsTableFrame.cpp
>   nsTableReflowState(const nsHTMLReflowState& aReflowState,
>-                     nscoord                  aAvailWidth,
>-                     nscoord                  aAvailHeight)
>+                     const LogicalSize& aAvailSize)
>     : reflowState(aReflowState)
>+    , availSize(aAvailSize)
>   {
>-    if (NS_UNCONSTRAINEDSIZE != availSize.width) {
>+    if (NS_UNCONSTRAINEDSIZE != availSize.ISize(wm)) {
[...]
>-    if (NS_UNCONSTRAINEDSIZE != availSize.height) {
>+    if (NS_UNCONSTRAINEDSIZE != availSize.BSize(wm)) {

Do we really need to worry about available ISize being unconstrained here? (I might be misremembering, but I thought only available-bsize could be unconstrained, while available ISize should always be an actual nscoord value.)

Note that the code being patched here -- which handles both width & height being unconstrained -- makes some sense, because either width or height could be in the block dimension. But if we're converting to logical units, and if we can assume (?) that available-isize must be constrained, then we can drop that first check.)

>@@ -2866,86 +2871,106 @@ nsTableFrame::SetupHeaderFooterChild(con
>                                      nscoord* aDesiredHeight)
> {
>   nsPresContext* presContext = PresContext();
>   nscoord pageHeight = presContext->GetPageSize().height;
> 
>   // Reflow the child with unconstrained height
>   WritingMode wm = aFrame->GetWritingMode();
>   LogicalSize availSize = aReflowState.reflowState.AvailableSize(wm);
>+  nscoord containerWidth = availSize.Width(wm);

We probably need to check if this gives us a NS_UNCONSTRAINEDSIZE containerWidth here, right? (which could happen if we have a vertical WM)  I think in other spots, you swap in 0 in that scenario, and do some fixup later on.

(For now, feel free to just stick in a XXX comment to this effect, and we'll probably need to fix it as we get thead/tfoot working in vertical writing-modes [assuming that this function is for thead/tfoot])

> void
> nsTableFrame::PlaceRepeatedFooter(nsTableReflowState& aReflowState,
>                                   nsTableRowGroupFrame *aTfoot,
>                                   nscoord aFooterHeight)
> {
>   nsPresContext* presContext = PresContext();
>   WritingMode wm = aTfoot->GetWritingMode();
>-  LogicalSize kidAvailSize(wm, aReflowState.availSize);
>+  LogicalSize kidAvailSize = aReflowState.availSize;
>+  nscoord containerWidth = kidAvailSize.Width(wm);

Same here. (need to consider kidAvailSize.Width(wm) being unconstrained here)

>   kidAvailSize.BSize(wm) = aFooterHeight;
>+  LogicalPoint kidPosition(wm, aReflowState.iCoord, aReflowState.bCoord);
[...]
>+  PlaceChild(aReflowState, aTfoot,
>+             kidPosition.GetPhysicalPoint(wm, containerWidth - desiredSize.Width()),
>+             desiredSize, origTfootRect, origTfootVisualOverflow);

Why the subtration from containerWidth here?  GetPhysicalPoint expects containerWidth, not containerWidth-minus-space-needed-for-this-element.

Please fix or add an explanatory comment.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8624209 [details] [diff] [review]
patch 2 - Convert nsTableFrame to work with logical coordinates. try: -b do -p linux64 -u all

Patch 2 review comments, "wave 2" (of 2):
=========================================

r=me with comments here & above addressed


> void
> nsTableFrame::ReflowChildren(nsTableReflowState& aReflowState,
[...]
>+  if (containerWidth == NS_UNCONSTRAINEDSIZE) {
>+    if (wm.IsVerticalRL()) {
[...]
>+    } else {
>+      NS_WARN_IF(containerWidth == NS_UNCONSTRAINEDSIZE &&
>+                 !wm.IsVertical() && !wm.IsBidiLTR());

Three things:
 (1) The containerWidth == NS_UNCONSTRAINEDSIZE check here inside the warning is trivially satisfied, per the "if" quoted above it here.

 (2) NS_WARN_IF isn't meant to be used like this -- you really want NS_WARN_IF_FALSE (note the FALSE) with an inverted condition. (NS_WARN_IF is evaluated in *both* debug and opt builds, because it's meant to wrap the contents of an "if" condition. It's not just a warning macro. See MXR for sample usage.)

 (3) It's not clear what this warning macro is trying to check.  I *suspect* you're trying to: (a) check that unconstrained width only happens in vertical WMs, and (b) check that we're not RTL, because we're about to use a bogus containerWidth and we're hoping it won't be used.  To make this all clearer, I'd say to handle "(a)" with a simple NS_WARN_IF_FALSE(wm.IsVertical(), ...) statement, just inside of the containerWidth==unconstrained check; and handle "(b)" with a simple code-comment. (Since at this point we know we're vertical and we're not vertical-RL, so explicitly checking & warning is maybe-sill.  Though if you want to do so, please include a warning message.)

>-          if (footerHeight + cellSpacingY < kidAvailSize.height) {
>+          if (footerHeight + cellSpacingY < kidAvailSize.BSize(wm)) {
[...]
>-      aReflowState.y += cellSpacingY;
>+      aReflowState.bCoord += cellSpacingY;

Please rename cellSpacingY to cellSpacingB here.

(This is in nsTableFrame::ReflowChildren.  Looks like this patch is renaming a similar local-variable in DistributeBSizeToRows(), but the version here in nsTableFrame::ReflowChildren() still has "Y" in its name & needs that same renaming action.)

>+              PlaceChild(aReflowState, kidFrame,
>+                         kidPosition.GetPhysicalPoint(wm, containerWidth -
>+                                                          desiredSize.Width()),

(Hmm, this is the same containerWidth - desiredSize.Width() thing that I was confused by at the end of "wave 1" here. Looks like this pattern happens again further down, too. Maybe I'm off-base on thinking this is wrong; please enlighten me?)

>+nsTableFrame::DistributeBSizeToRows(const nsHTMLReflowState& aReflowState,
>+                                    nscoord                  aAmount)
[...]
>-    nscoord yOriginRow = 0;
>+    nscoord bOriginRow = 0;
[...]
>           if (amountForRow > 0) {
>             // XXXbz we don't need to move the row's y position to yOriginRow?
>             nsRect origRowRect = rowFrame->GetRect();

Please update the XXXbz comment there to be in sync with the code. ("y position", "yOriginRow" both need to say "b" now)

>-            rowFrame->SetSize(nsSize(rowNormalRect.width, newRowHeight));
>+            rowFrame->SetSize(nsSize(rowNormalRect.ISize(wm), newRowBSize));

This looks mismatched -- we're using the physical SetSize method (with a physical nsSize), but we're passing in a logical ISize and a BSize as our coords.

I think you want something like:
              rowFrame->SetSize(wm, LogicalSize(wm, rowNormalRect.ISize(wm),
                                                newRowBSize));

>-            rowFrame->MovePositionBy(nsPoint(0, yOriginRow - rowNormalRect.y));
>+            rowFrame->MovePositionBy(wm, LogicalPoint(wm, 0, bOriginRow -
>+                                                    rowNormalRect.BStart(wm)));
[...]
>+        rgFrame->SetSize(wm, LogicalSize(wm, rgNormalRect.ISize(wm),
>+                                rgNormalRect.BSize(wm) + amountUsedByRG));

Tangent (no action needed): in both calls here, the last line ends up awkwardly/arbitrarily deindented (presumably to avoid going over 80 characters).

I was going to suggest pulling the LogicalPoint/LogicalSize here out into a local variable...  but it looks like this same pattern happens several times below this point (without the need for the awkward de-indentation), and it's probably worth keeping this pattern in sync among these various places. So, I suppose this awkward-deindentation is fine for now, in that interest.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8624210 [details] [diff] [review]
patch 3 - Rename a couple more frame-state bits from physical to logical. try: -b do -p linux64 -u all

>diff --git a/layout/generic/nsFrameStateBits.h b/layout/generic/nsFrameStateBits.h
> // Indicates whether this row has any cells that have
> // non-auto-height and rowspan=1
>-FRAME_STATE_BIT(TableRow, 29, NS_ROW_HAS_CELL_WITH_STYLE_HEIGHT)
>+FRAME_STATE_BIT(TableRow, 29, NS_ROW_HAS_CELL_WITH_STYLE_BSIZE)

s/height/bsize/ in the comment here.

>+++ b/layout/tables/nsTableCellFrame.h
> inline bool nsTableCellFrame::HasPctOverBSize()
> {
>-  return (mState & NS_TABLE_CELL_HAS_PCT_OVER_HEIGHT) ==
>-         NS_TABLE_CELL_HAS_PCT_OVER_HEIGHT;
>+  return (mState & NS_TABLE_CELL_HAS_PCT_OVER_BSIZE) ==
>+         NS_TABLE_CELL_HAS_PCT_OVER_BSIZE;
[etc]

(FWIW: I filed bug 1176555 on replacing the bitwise arithmetic here with our more concise, readable nsIFrame state-manipulation methods)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8624220 [details] [diff] [review]
patch 4 - Rename mSpecialHeightReflow to mSpecialBSizeReflow, and update comments to match

r=me on patch 4, just one nit:

>+++ b/layout/generic/nsHTMLReflowState.h
>   struct ReflowStateFlags {
>-    uint16_t mSpecialHeightReflow:1; // used by tables to communicate special reflow (in process) to handle
>+    uint16_t mSpecialBSizeReflow:1; // used by tables to communicate special reflow (in process) to handle
>                                      // percent height frames inside cells which may not have computed heights

sed broke the comment alignment here. (since "BSize" is 1 character shorter than "Height").

Add an extra space between "mSpecialBSizeReflow:1;" and the comment, to keep the comment aligned with its next line (and all the comments below it).

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92ce2f691019
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5cf0bdd0c1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca41ad40687f
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc622672848

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92ce2f691019
https://hg.mozilla.org/mozilla-central/rev/f5cf0bdd0c1a
https://hg.mozilla.org/mozilla-central/rev/ca41ad40687f
https://hg.mozilla.org/mozilla-central/rev/dbc622672848

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: patch 5 - Copy inline-size rather than width from prev-in-flow when initializing nsTableFrame

Here's a minor followup that should really have been part of the main patch (2) here.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8625927 [details] [diff] [review]
patch 5 - Copy inline-size rather than width from prev-in-flow when initializing nsTableFrame

Looks good, thanks!

Comment 17

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b47603f369df

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b47603f369df