Closed Bug 1174700 Opened 9 years ago Closed 9 years ago

convert nsTableRowFrame and nsTableRowGroupFrame to work with logical coordinates

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

patch 2 - Convert nsTableFrame::SetColumnDimensions to work with logical-order iteration over the table, so that column coordinates and collapsed borders are correct
6.74 KB, patch
Details | Diff | Splinter Review
Convert nsTableRowFrame and nsTableRowGroupFrame to work with logical coordinates
157.86 KB, patch
Details | Diff | Splinter Review 
Another step towards vertical tables. (None of this should have any visible effect on behavior until we enable it at the top level of table layout.)
Blocks: 1174711
Sorry, this is kinda huge, but nsTableRowFrame and nsTableRowGroupFrame work closely enough together that it didn't really make sense to try and tackle them in pieces. AFAICS this passes all current unit tests; however, it actually breaks the rendering of collapsed borders with direction:rtl, so we clearly don't have adequate test coverage there. The basic issue is that I've changed nsTableIterator such that it always iterates in 'forward' logical order, instead of iterating backwards over RTL tables, but the collapsed-border code hasn't been adapted for that yet; I'm aiming to do that in a later patch.
Attachment #8623036 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(In reply to Jonathan Kew (:jfkthame) from comment #3)
>  AFAICS this passes all current unit tests; however, it
> actually breaks the rendering of collapsed borders with direction:rtl, so we
> clearly don't have adequate test coverage there.

I've just posted a reftest in bug 1157569 that would detect this breakage. The issue here arises because column dimensions end up backwards, now that nsTableIterator runs logically instead of physically through the RTL table. This can also be seen by looking at the rects of <col> and <colgroup> elements using the Element Inspector.
This fixes nsTableFrame::SetColumnDimensions so that it works properly now that we no longer iterate backwards over RTL rows; with this, the column rects look good in the Inspector, and collapsed borders render properly again (modulo the issue in bug 1157569 comment 42).
Attachment #8623555 - Flags: review?(dholbert)
The first patch here doesn't apply cleanly to current mozilla-central -- does it depend on your patches in some other bug(s)?

(I'd ideally like to use a merge tool to help with review - that makes search-and-replace type patches much easier to sanity-check, since my merge tool highlights the changed parts of each edited line. But for that to work, I have to get the patch to apply cleanly)
Flags: needinfo?(jfkthame)
Comment on attachment 8623036 [details] [diff] [review]
Convert nsTableRowFrame and nsTableRowGroupFrame to work with logical coordinates. try: -b do -p linux64 -u all

Initial review comments, from skimming a .rej file for part of the patch that didn't apply cleanly (in nsTableRowGroupFrame.cpp):

>+++ b/layout/tables/nsTableRowGroupFrame.cpp
> static void
> CacheRowHeightsForPrinting(nsPresContext*   aPresContext,
>                            nsTableRowFrame* aFirstRow)
> {
>+  WritingMode wm = aFirstRow->GetWritingMode();
>   for (nsTableRowFrame* row = aFirstRow; row; row = row->GetNextRow()) {
[...]
>-      row->SetHasUnpaginatedHeight(true);
>-      row->SetUnpaginatedHeight(aPresContext, row->GetSize().height);
>+      row->SetHasUnpaginatedBSize(true);
>+      row->SetUnpaginatedBSize(aPresContext, row->GetLogicalSize(wm).BSize(wm));

3 things:
 (1) The second arg here can be condensed to just row->BSize(wm).
 (2) This function probably needs s/Height/BSize/ in its name.
 (3) Rather than making the virtual function-call to look up the wm, this function should probably just take an "aWM" arg. (It only has 2 callers, and each has a reflow state in-scope that they can cheaply pull the WM off of.)

>-  // XXXldb Should we really be checking this rather than available height?
>+  // XXXldb Should we really be checking this rather than available block-size?
>   // (Think about multi-column layout!)
>   bool isPaginated = aPresContext->IsPaginated() &&
>-                       NS_UNCONSTRAINEDSIZE != aReflowState.availSize.height;
>+                       NS_UNCONSTRAINEDSIZE != aReflowState.availSize.BSize(wm);

This comment makes no sense nowadays, because we *do* check the height/BSize.

The comment is here because it predates that availabe-height check; the check was added here, without updating the comment:
  http://hg.mozilla.org/mozilla-central/rev/fcff7673a6ae#l3.10

I think nowadays this comment really wants to say:
  // XXXldb Should we really be checking IsPaginated(),
  // or should we *only* check available block-size?

Please update the comment to say something like that ^^ (here or in a followup).

>+      nscoord containerWidth = aReflowState.reflowState.ComputedWidth();
>+      if (containerWidth == NS_UNCONSTRAINEDSIZE) {
>+        containerWidth = 0;
>+      }
>+      LogicalRect oldKidRect = kidFrame->GetLogicalRect(wm, containerWidth);

|containerWidth| is supposed to be the border-box size, but reflowState.ComputedWidth() is the content-box size. Normally we're supposed to add borderpadding here -- except I think here we know there won't be borderpadding, because maybe rowgroups can't have border or padding (?)

Please add a comment (or an assertion) to mention that borderpadding is 0 here which is why we're not bothering with it.
Refreshed patch and fixed a bunch more comments, naming, etc. This should apply on current inbound (on top of the landed patches for bugs 1174507 and 1173307).
Attachment #8624140 - Flags: review?(dholbert)
Attachment #8623036 - Attachment is obsolete: true
Attachment #8623036 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #8)
> >+      nscoord containerWidth = aReflowState.reflowState.ComputedWidth();
> >+      if (containerWidth == NS_UNCONSTRAINEDSIZE) {
> >+        containerWidth = 0;
> >+      }
> >+      LogicalRect oldKidRect = kidFrame->GetLogicalRect(wm, containerWidth);
> 
> |containerWidth| is supposed to be the border-box size, but
> reflowState.ComputedWidth() is the content-box size. Normally we're supposed
> to add borderpadding here -- except I think here we know there won't be
> borderpadding, because maybe rowgroups can't have border or padding (?)
> 
> Please add a comment (or an assertion) to mention that borderpadding is 0
> here which is why we're not bothering with it.

I thought this was the case, but it turns out this doesn't hold when border-collapse is used, so instead I've added the borderPadding here.
Flags: needinfo?(jfkthame)
BTW, one thing I've left unconverted for now is the code in nsTableRowGroupFrame for splitting rowGroups (during pagination). I've filed bug 1175859 as a followup to address that.
Minor update to fix up some more comments, spacing, etc.
Attachment #8624178 - Flags: review?(dholbert)
Attachment #8624140 - Attachment is obsolete: true
Attachment #8624140 - Flags: review?(dholbert)
Comment on attachment 8623555 [details] [diff] [review]
patch 2 - Convert nsTableFrame::SetColumnDimensions to work with logical-order iteration over the table, so that column coordinates and collapsed borders are correct. try: -b do -p linux64 -u all

(Started reviewing "patch 2" first, since it's smaller)

r=me with nits below addressed as you see fit:

>+++ b/layout/tables/nsTableFrame.cpp
> void
>-nsTableFrame::SetColumnDimensions(nscoord aHeight, WritingMode aWM,
>+nsTableFrame::SetColumnDimensions(nscoord aBSize, WritingMode aWM,
>                                   const LogicalMargin& aBorderPadding)
> {
[...]
>+  nscoord containerWidth = mRect.width;
[...]
>+    colGroupFrame->SetRect(aWM, colGroupRect, containerWidth);

I don't think this is quite right. We're called from nsTableFrame::Reflow, which is before our mRect has received its final size.

We should probably make this method take a new parameter, aContainerWidth, and have the caller pass in aDesiredSize.Width() (which the caller has just finalized).

>-  nscoord colHeight = aHeight -= aBorderPadding.BStartEnd(aWM) +
>-                                 GetRowSpacing(-1) +
>-                                 GetRowSpacing(GetRowCount());
>-
>+  nscoord colBSize = aBSize -= aBorderPadding.BStartEnd(aWM) +
>+                               GetRowSpacing(-1) +
>+                               GetRowSpacing(GetRowCount());

The "= ... -=" dance is needlessly complex here. aBSize is never used after this point, so there's really no need to modify it.

So: please convert "-= ...;" to "- (...);"

(Also: consider declaring colBSize as 'const', since I think it might be the only nscoord in this function which is held constant.)

>+  int32_t colI = 0;
>-  int32_t colX =tableIsLTR ? 0 : std::max(0, GetColCount() - 1);

Not sure s/X/I/ makes sense here; I think this is the column *index* (not an inline coord or anything).  "colI" makes me think it's the inline coord of a column (particularly when mixed with nscoords named colGroupISize, cellSpacingI, etc.)

So: consider naming this "colIdx"? (seems to be a 0-based index).

>-  while (colGroupFrame) {
>+  for (nsIFrame* colGroupFrame = iter.First(); colGroupFrame;
>+       colGroupFrame = iter.Next()) {
[...]
>     nsTableIterator iterCol(*colGroupFrame);
>     nsIFrame* colFrame = iterCol.First();
>-    nsPoint colOrigin(0,0);
>     while (colFrame) {

You've converted the outer loop from "while" to "for", which is a nice conversion.  Could you do that for both inner loops, too? (first inner loop quoted here)

>+    LogicalRect colGroupRect(aWM, colGroupOrigin.I(aWM), colGroupOrigin.B(aWM),
>+                             colGroupISize, colBSize);
>+    colGroupFrame->SetRect(aWM, colGroupRect, containerWidth);
>+    colGroupOrigin.I(aWM) += colGroupISize + cellSpacingI;
>+
>+    // then we can place the columns correctly within the group
>+    colI = groupFirstCol;

Consider moving the "colGroupOrigin.I(aWM) += colGroupISize + cellSpacingI;" line down by ~16 lines, to be the final line of the outer loop here.  (I think this line is prepping for the next colgroup [i.e. for the next outer-loop-iteration]. But right now, it happens halfway through, while we're still dealing with this colgroup, so it's not as obvious [in its current context] why we're making this adjustment.)

>+    LogicalPoint colOrigin = LogicalPoint(aWM);

This syntax seems odd. Can't we just do:
  LogicalPoint colOrigin(aWM);

>+    while (colFrame) {
>+      if (NS_STYLE_DISPLAY_TABLE_COLUMN ==
>+          colFrame->StyleDisplay()->mDisplay) {
>+        nscoord colISize = GetColumnISize(colI);

(note: each call to GetColumnISize() involves one or more virtual function calls to FirstInFlow, which we should really coalesce. I filed bug 1176070 on this.)
Attachment #8623555 - Flags: review?(dholbert) → review+
Comment on attachment 8624178 [details] [diff] [review]
Convert nsTableRowFrame and nsTableRowGroupFrame to work with logical coordinates

Review notes, wave 1
====================

>diff --git a/layout/tables/nsTablePainter.cpp b/layout/tables/nsTablePainter.cpp
> TableBackgroundPainter::PaintRowGroup(nsTableRowGroupFrame* aFrame,
>                                       TableBackgroundData   aRowGroupBGData,
>                                       bool                  aPassThrough)
> {
[...]
>   nsTableRowFrame* firstRow = aFrame->GetFirstRow();
[...]
>+      LogicalMargin border(wm);
>       if (firstRow) {
>         //pick up first row's top border (= rg top border)
>-        firstRow->GetContinuousBCBorderWidth(border);
>+        nsMargin b;
>+        firstRow->GetContinuousBCBorderWidth(b);
>+        border = LogicalMargin(wm, b);
[...]
>+      aFrame->GetContinuousBCBorderWidth(wm, border);
>+      aRowGroupBGData.SetBCBorder(border.GetPhysicalMargin(wm));

So it looks like, as of this patch:
 nsTableRowFrame::GetContinuousBCBorderWidth takes a nsMargin.
 nsTableRowGroupFrame::GetContinuousBCBorderWidth takes a LogicalMargin.

Is there a reason for that distinction? Do we alreayd have a plan to logicalize the row version? (doesn't necessarily have to be part of this patch; but it'll remove the need for the "nsMargin b" dummy-variable that you're having to rely on here, I think.)

>+++ b/layout/tables/nsTableRowFrame.cpp
> nscoord
>-GetHeightOfRowsSpannedBelowFirst(nsTableCellFrame& aTableCellFrame,
>-                                 nsTableFrame&     aTableFrame)
>+GetBSizeOfRowsSpannedBelowFirst(nsTableCellFrame& aTableCellFrame,
>+                                nsTableFrame&     aTableFrame,
>+                                const WritingMode aWM)
> {
>-  nscoord height = 0;
>+  nscoord bsize = 0;
>   int32_t rowSpan = aTableFrame.GetEffectiveRowSpan(aTableCellFrame);
>   // add in height of rows spanned beyond the 1st one

s/height/bsize/ in comment

>@@ -325,24 +328,26 @@ nsTableRowFrame::DidResize()
>     if (cellFrame) {
>-      nscoord cellHeight = mRect.height + GetHeightOfRowsSpannedBelowFirst(*cellFrame, *tableFrame);
>+      nscoord cellBSize = BSize(wm) +
>+        GetBSizeOfRowsSpannedBelowFirst(*cellFrame, *tableFrame, wm);

Up higher in this function (nsTableRowFrame::DidResize), on its 1st line, there's the following comment:
  // Resize and re-align the cell frames based on our row height
...which is referring to what we're doing here.

That comment needs s/height/BSize/.
 
>-      // resize the cell's height
>-      nsRect cellRect = cellFrame->GetRect();
>+      // resize the cell's bsize
[...]
>+      if (cellSize.BSize(wm) != cellBSize) {
>+        cellSize.BSize(wm) = cellBSize;
>+        nsRect cellRect = cellFrame->GetRect();
>+        cellFrame->SetSize(wm, cellSize);
>         nsTableFrame::InvalidateTableFrame(cellFrame, cellRect,

Nit: please rename "cellRect" to "cellOldRect", or something similar.

(Your patch is narrowing the usage of this variable.  Now, it simply saves something that we're about to stomp on, for usage after we've stomped on it.  "cellOldRect" captures this usage better than "cellRect", IMO.)

> nscoord
>-nsTableRowFrame::GetHeight(nscoord aPctBasis) const
>+nsTableRowFrame::GetBSize(nscoord aPctBasis) const
> {

Hmm, so this is a problem -- nsTableRowFrame will now have two easy-to-confuse methods: "BSize(wm)" and "GetBSize(aPctBasis)". Both return nscoord, and their names are almost the same (they just differ by "Get"), but they check completely different data & do completely different things.

We should not remain in this confusing state indefinitely.  Maybe GetBSize() should to be renamed to "GetTentativeBSize()"? Please file a followup to cover this, or address it here if you can think of a better name.


>+nsTableRowFrame::UpdateBSize(nscoord           aBSize,
>+                             nscoord           aAscent,
>+                             nscoord           aDescent,
>+                             nsTableFrame*     aTableFrame,
>+                             nsTableCellFrame* aCellFrame)
> {
[...]
>     else { // the alignment on the baseline can change the height

comment needs s/height/bsize/

>       // keep the tallest height in sync
>-      if (GetHeight() < mMaxCellAscent + mMaxCellDescent) {
>-        SetContentHeight(mMaxCellAscent + mMaxCellDescent);
>+      if (GetBSize() < mMaxCellAscent + mMaxCellDescent) {
>+        SetContentBSize(mMaxCellAscent + mMaxCellDescent);

comment needs s/height/bsize/

> nscoord
>-nsTableRowFrame::CalcHeight(const nsHTMLReflowState& aReflowState)
>+nsTableRowFrame::CalcBSize(const nsHTMLReflowState& aReflowState)
> {
[...]
>       // height may have changed, adjust descent to absorb any excess difference
[...]
>-       UpdateHeight(desSize.BSize(wm), ascent, descent, tableFrame, cellFrame);
>+       UpdateBSize(desSize.BSize(wm), ascent, descent, tableFrame, cellFrame);

comment needs s/height/bsize/

>+nsTableRowFrame::CalculateCellActualBSize(nsTableCellFrame* aCellFrame,
>+                                          nscoord&          aDesiredBSize,
>+                                          WritingMode       aWM)
> {
>-  nscoord specifiedHeight = 0;
>+  nscoord specifiedBSize = 0;
> 
>-  // Get the height specified in the style information
>+  // Get the bsize specified in the style information
>   const nsStylePosition* position = aCellFrame->StylePosition();
> 
>   int32_t rowSpan = GetTableFrame()->GetEffectiveRowSpan(*aCellFrame);
> 
>-  switch (position->mHeight.GetUnit()) {
>+  const nsStyleCoord& bsizeCoord = position->BSize(aWM);

The name "bsizeCoord" is a bit confusing here, since "coord" is such an overloaded[1] term. Maybe rename to "bsizeStyle"? or "bsizeStyleCoord"?

[1] Examples of overloadedness in this section of code:
 - lower down, we check (in a switch statement) if this var "bsizeCoord" has unit "eStyleUnit_Coord" (and from its name, one might expect that it surely has that unit)! But no, these are just different meanings of "Coord".
 - And again, when we pass it to ComputeCoordPercentCalc, one might expect that really it's guaranteed to have the "Coord" flavor. (but no)
 - And there are various nscoord values in play, which are also using "coord" in a different sense than bsizeCoord.


> static
>-nscoord CalcHeightFromUnpaginatedHeight(nsPresContext*   aPresContext,
>-                                        nsTableRowFrame& aRow)
>+nscoord CalcBSizeFromUnpaginatedBSize(nsPresContext*   aPresContext,
>+                                      nsTableRowFrame& aRow)
> {
>-  nscoord height = 0;
>+  nscoord bsize = 0;
>+  WritingMode wm = aRow.GetWritingMode();

Promote wm to be a parameter here, so we don't have to look it up (and pay for a virtual function call).

This function only has two callers, both in ReflowChildren(), so they can grab it off of a reflow state.

>+++ b/layout/tables/nsTableRowFrame.h
>   // calculate the height, considering content height of the 
>   // cells and the style height of the row and cells, excluding pct heights
>-  nscoord CalcHeight(const nsHTMLReflowState& aReflowState);
>+  nscoord CalcBSize(const nsHTMLReflowState& aReflowState);

Update comment (s/height/bsize/)

> private:
>   struct RowBits {
>     unsigned mRowIndex:29;
>-    unsigned mHasFixedHeight:1; // set if the dominating style height on the row or any cell is pixel based
>-    unsigned mHasPctHeight:1;   // set if the dominating style height on the row or any cell is pct based
>-    unsigned mFirstInserted:1;  // if true, then it was the top most newly inserted row 
>+    unsigned mHasFixedBSize:1; // set if the dominating style height on the row or any cell is pixel based
>+    unsigned mHasPctBSize:1;   // set if the dominating style height on the row or any cell is pct based

Comments here need s/height/bsize/ (in "dominating style height")
> Hmm, so this is a problem -- nsTableRowFrame will now have two
> easy-to-confuse methods: "BSize(wm)" and "GetBSize(aPctBasis)". Both return
> nscoord, and their names are almost the same (they just differ by "Get"),
> but they check completely different data & do completely different things.
> 
> We should not remain in this confusing state indefinitely.  Maybe GetBSize()
> should to be renamed to "GetTentativeBSize()"? Please file a followup to
> cover this, or address it here if you can think of a better name.

How about GetInitialBSize(), as it's the initial size before we've distributed extra bsize to rows, etc?

But let's settle on a name and fix it in a followup bug, as I think it'll conflict with the patches currently in bug 1174711.
This is a followup patch that applies on top of attachment 8624178 [details] [diff] [review] to address the Wave 1 comments (thanks for all the great reviews, btw), except for renaming GetBSize() which I'd prefer to defer to a followup just to reduce churn in other patches.
Attachment #8624757 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #15)

> So it looks like, as of this patch:
>  nsTableRowFrame::GetContinuousBCBorderWidth takes a nsMargin.
>  nsTableRowGroupFrame::GetContinuousBCBorderWidth takes a LogicalMargin.
> 
> Is there a reason for that distinction?

Only that we're in an incompletely-converted state at this point. Anyhow, in the followup (above) I've converted the nsTableRowFrame version as well, though we still have physical versions of this method on nsTableColFrame and nsTableColGroupFrame; and nsTablePainter (which uses them) is still largely physical.
Attachment #8623555 - Attachment is obsolete: true
(In reply to Jonathan Kew (:jfkthame) from comment #16)
> How about GetInitialBSize(), as it's the initial size before we've
> distributed extra bsize to rows, etc?
> 
> But let's settle on a name and fix it in a followup bug, as I think it'll
> conflict with the patches currently in bug 1174711.

That sounds good.

"patch 1a" looks good, too - thanks! (I assume you intend to fold it into part 1 when landing? That'll make some of the conversions around GetContinuousBCBorderWidth a bit more direct [it'll remove intermediate nsMargin local-vars that you add in part 1 & remove in part 1a], so it's probably cleaner that way. Up to you though.)
Attachment #8624757 - Flags: review?(dholbert) → review+
Blocks: 1176354
Comment on attachment 8624178 [details] [diff] [review]
Convert nsTableRowFrame and nsTableRowGroupFrame to work with logical coordinates

Review notes, wave 2: (getting to the end of nsTableRowFrame)
=====================


>+++ b/layout/tables/nsTableRowFrame.cpp
> void
>-nsTableRowFrame::ReflowChildren(nsPresContext*          aPresContext,
>+nsTableRowFrame::ReflowChildren(nsPresContext*           aPresContext,
[...]
>     // remember the rightmost (ltr) or leftmost (rtl) column this cell spans into
>-    prevColIndex = (iter.IsLeftToRight()) ? cellColIndex + (cellColSpan - 1) : cellColIndex;
>+    prevColIndex = cellColIndex + (cellColSpan - 1);

Comment-tweak needed here, both to remove the ltr/rtl distinction and the implication that we're horizontal.

Something like:
 // Remember the istart-most column this cell spans into.

>     nsRect kidRect = kidFrame->GetRect();
>-    nsPoint origKidNormalPosition = kidFrame->GetNormalPosition();
>-    MOZ_ASSERT(origKidNormalPosition.y == 0);
>+    LogicalPoint origKidNormalPosition =
>+      kidFrame->GetLogicalNormalPosition(wm, containerWidth);
>+    MOZ_ASSERT(origKidNormalPosition.B(wm) == 0 || wm.IsVerticalRL());

This assertion (particularly the new wm.IsVerticalRL() special-case) is confusing.

I think the assertion is basically saying:
 - All cells' no-relative-positioning position should be snapped to the row's bstart edge.
except I don't understand why wm.IsVerticalRL() needs an exception.

Please add a comment and/or assertion-message to make it clearer what we're asserting here, so less digging is required to grok this.

>-      // we need to account for the cell's width even if it isn't reflowed
>-      x += kidRect.width;
>+      // we need to account for the cell's isize even if it isn't reflowed
>+      iCoord += LogicalSize(wm, kidRect.Size()).ISize(wm);

Consider shortening that last line to just:
  iCoord += kidFrame.ISize(wm);

(This is equivalent because kidRect is just a copy of kidFrame->GetRect(), and neither copy's size has changed, because we didn't reflow this child -- we just repositioned it. So it doesn't much matter whether we use the local copy of this rect vs. the copy that lives on kidFrame.)

>+    // Any children whose width was not the same as our final
>+    // aDesiredSize.BSize will have been misplaced earlier at the
>+    // FinishReflowChild stage. So fix them up now.
[...]
>+      if (kidFrame->BSize(wm) != aDesiredSize.BSize(wm)) {
>+        kidFrame->MovePositionBy(wm,
>+          LogicalPoint(wm, 0, kidFrame->BSize(wm) - aDesiredSize.BSize(wm)));

Don't we also need a call to nsTableFrame::RePositionViews(kidFrame) here?

At least: higher up, we call RePositionViews after a different call to MovePositionBy.

>-nsTableRowFrame::Reflow(nsPresContext*          aPresContext,
>+nsTableRowFrame::Reflow(nsPresContext*           aPresContext,
>                         nsHTMLReflowMetrics&     aDesiredSize,
>                         const nsHTMLReflowState& aReflowState,
>                         nsReflowStatus&          aStatus)
> {
>   // See if we have a cell with specified/pct height
>-  InitHasCellWithStyleHeight(tableFrame);
>+  InitHasCellWithStyleBSize(tableFrame);

s/height/bsize/ in comment

> 
>   ReflowChildren(aPresContext, aDesiredSize, aReflowState, *tableFrame, aStatus);
> 
>   if (aPresContext->IsPaginated() && !NS_FRAME_IS_FULLY_COMPLETE(aStatus) &&
>       ShouldAvoidBreakInside(aReflowState)) {
>     aStatus = NS_INLINE_LINE_BREAK_BEFORE();
>   }
> 
>   // just set our width to what was available. The table will calculate the width and not use our value.
>-  aDesiredSize.Width() = aReflowState.AvailableWidth();
>+  aDesiredSize.ISize(wm) = aReflowState.AvailableISize();

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

> /**
>  * This function is called by the row group frame's SplitRowGroup() code when
>  * pushing a row frame that has cell frames that span into it. The cell frame
>  * should be reflowed with the specified height
>  */
> nscoord
>-nsTableRowFrame::ReflowCellFrame(nsPresContext*          aPresContext,
>+nsTableRowFrame::ReflowCellFrame(nsPresContext*           aPresContext,

s/height/bsize/ at end of comment here.

> nscoord
> nsTableRowFrame::CollapseRowIfNecessary(nscoord aRowOffset,
[...]
>-  nsRect rowRect = GetRect();
>-  nsRect oldRect = rowRect;
>+  WritingMode wm = GetWritingMode();
>+  nscoord containerWidth = mRect.width;
>+
>+  LogicalRect rowRect = GetLogicalRect(wm, containerWidth);

'containerWidth' has the wrong value for this particular call. It's 'this' frame's width, and this->GetLogicalRect() really wants our *container's* width.

I think we really need to get our table frame & use its width here (I think that's what our physical mRect is with respect to, at least -- that's what matters for this API).

However, 'containerWidth' is used several more times in this function, for positioning table-cells -- and the current patch's value *is* correct for those usages, I think. So we probably need two different containerWidth variables here.

>     nscoord x = 0; // running total of children x offset

I think this wants to be named "iPos" or something (and s/x/inline-axis/ in comment)

>-            cRect.height += nextRect.height +
>+            cRect.BSize(wm) += nextRect.BSize(wm) +
>                             tableFrame->GetRowSpacing(rowFrame->GetRowIndex());

Fix up indentation on the last line here. (indent tableFrame by 3 more spaces)
(In reply to Daniel Holbert [:dholbert] from comment #21)
> 'containerWidth' has the wrong value for this particular call. It's 'this'
> frame's width, and this->GetLogicalRect() really wants our *container's*
> width.
> 
> I think we really need to get our table frame & use its width here

Er, correcting myself: looking at a frame dump, it looks like we'd want the *rowgroup frame's* width here (not the table's).  Looks like table rows' mRects are positioned w.r.t. their rowgroup, which makes sense.

(Also: in horizontal writing-modes, the rowgroup's width will be the same as the row's width, so your current patch will Just Work. But in a vertical writing-mode (where the width is the block-axis), the row's width (bsize) won't be the same as its rowgroup (unless it's the only row).  In that scenario, I'd expect this containerWidth issue to produce bugs.)
Whiteboard:
Whiteboard: 
Comment on attachment 8624178 [details] [diff] [review]
Convert nsTableRowFrame and nsTableRowGroupFrame to work with logical coordinates

Review notes, wave 3 of 3:
==========================

r=me with notes in comment 21 (/22) & this comment addressed as you see fit.

>+++ b/layout/tables/nsTableRowGroupFrame.cpp
> // Position and size aKidFrame and update our reflow state. The origin of
> // aKidRect is relative to the upper-left origin of our frame
> void
> nsTableRowGroupFrame::PlaceChild(nsPresContext*         aPresContext,

Drop the second sentence of this comment. Nothing is relative to the upper-left origin of our frame, and aKidRect hasn't actually been the name of any parameter here here for quite some time.

(though it was when this comment was added in 1999:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowGroupFrame.cpp&rev=3.107#259

> void
> nsTableRowGroupFrame::ReflowChildren(nsPresContext*         aPresContext,
[...]
>+      // bsizes, taking into account cells with row spans...
>+      LogicalSize kidAvailSize = aReflowState.availSize;
>       kidAvailSize.BSize(wm) = NS_UNCONSTRAINEDSIZE;
>       nsHTMLReflowState kidReflowState(aPresContext, aReflowState.reflowState,
>                                        kidFrame, kidAvailSize,
>                                        nullptr,
>                                        nsHTMLReflowState::CALLER_WILL_INIT);
>       InitChildReflowState(*aPresContext, borderCollapse, kidReflowState);
> 
>       // This can indicate that columns were resized.

Right after this comment (outside of the context of this patch), there's some more code that I think needs logicalizing here. Specifically:
      if (aReflowState.reflowState.IsHResize()) {
        kidReflowState.SetHResize(true);
      }
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowGroupFrame.cpp?rev=2f789a4def0a#382

I suspect this needs to change to IsIResize() / SetIResize()?

(Feel free to punt to a separate bug/patch if you think this belongs elsewhere.)


>@@ -387,82 +401,84 @@ nsTableRowGroupFrame::ReflowChildren(nsP
>       // Place the child
>-      PlaceChild(aPresContext, aReflowState, kidFrame, kidPosition,
>-                 desiredSize, oldKidRect, oldKidVisualOverflow);
>-      aReflowState.y += cellSpacingY;
>+      PlaceChild(aPresContext, aReflowState, kidFrame,
>+                 wm, kidPosition, containerWidth,
>+                 desiredSize, oldKidRect.GetPhysicalRect(wm, containerWidth), oldKidVisualOverflow);

Wrap before "oldKidVisualOverflow" on that last line.

>       // Adjust the running y-offset so we know where the next row should be placed
>-      nscoord height = kidFrame->GetSize().height + cellSpacingY;
>-      aReflowState.y += height;
>+      nscoord bSize = kidFrame->BSize(wm) + cellSpacingB;
>+      aReflowState.bCoord += bSize;

s/y-offset/b-offset/ in comment

>+  if (wm.IsVerticalRL()) {
>+    // we need the correct containerWidth below for block positioning in
>+    // vertical-rl writing mode
>+    containerWidth = rowGroupBSize;
>+  }

Consider relaxing this check to just IsVertical() here, to make this a less mysteriously special-casey.

Technically, we know that containerWidth will only *matter* if we're RTL-flavored in the horizontal axis.  But the code that acts on it is abstracted away several layers, and this assignment is cheap, and it seems nice to keep things simple and avoid single-writing-mode special-cases.

(We technically *could* make all containerWidth assignments depend on RTL-ness, but I don't think we do, and I assume that's to avoid confusing proliferation of special-casing.)


> nscoord
>-nsTableRowGroupFrame::CollapseRowGroupIfNecessary(nscoord aYTotalOffset,
>-                                                  nscoord aWidth)
>+nsTableRowGroupFrame::CollapseRowGroupIfNecessary(nscoord aBTotalOffset,
>+                                                  nscoord aISize)
> {
>+  WritingMode wm = GetWritingMode(); // XXX pass from caller

Did you meant to leave this XXX comment in?  (Looks like there's only one caller -- nsTableFrame::AdjustForCollapsingRowsCols -- and it already has "aWM", which it can pass to us. So, shouln't be too hard to address it here.)

>   nsTableFrame* tableFrame = GetTableFrame();
>+  nscoord containerWidth = tableFrame->GetRect().width;

I think this containerWidth value is stale. This function (CollapseRowGroupIfNecessary) is called while the tableFrame is being reflowed, so it's about to get a new & different width (which is actually currently being adjusted).

Maybe file a followup to cover this and drop an XXX comment here pointing to that followup? I'm not sure what the right containerWidth to use here is. Our caller (AdjustForCollapsingRowsCols) is in the process of updating the table's aDesiredSize, so it could pass us aDesiredSize.Width() as *some* relatively up-to-date containerWidth, but that's not really right, because it could still end up changing after that.

(This might be a case where we need to pretend containerWidth is 0, and then do a second pass after we've established the final size of the container to move children into their final position...)

>-void nsTableRowGroupFrame::SetContinuousBCBorderWidth(uint8_t     aForSide,
>+void nsTableRowGroupFrame::SetContinuousBCBorderWidth(LogicalSide aForSide,
>                                                       BCPixelSize aPixelValue)
> {
>   switch (aForSide) {
[...]
>-    case NS_SIDE_LEFT:
>-      mLeftContBorderWidth = aPixelValue;
>+    case eLogicalSideIStart:
>+      mIStartContBorderWidth = aPixelValue;
>       return;
>     default:
>       NS_ERROR("invalid NS_SIDE argument");

NS_ERROR message needs an update -- s/NS_SIDE/LogicalSide/

>+++ b/layout/tables/nsTableRowGroupFrame.h
>   /**
>     * Adjust to the effect of visibibility:collapse on the row group and
>     * its children
>     * @return              additional shift upward that should be applied to
>     *                      subsequent rowgroups due to rows and this rowgroup
>     *                      being collapsed
>     * @param aYTotalOffset the total amount that the rowgroup is shifted up
>     * @param aWidth        new width of the rowgroup
>     */
>-  nscoord CollapseRowGroupIfNecessary(nscoord aYTotalOffset,
>-                                      nscoord aWidth);
>+  nscoord CollapseRowGroupIfNecessary(nscoord aBTotalOffset,
>+                                      nscoord aISize);

s/upwards/bstart-wards/ in comment

>@@ -308,17 +311,17 @@ public:
>    * The actual row returned might not contain 'aY', but if not, it is guaranteed
>    * to be before any row which does contain 'aY'.
>    * aOverflowAbove is the maximum over all rows of -row.GetOverflowRect().y.
>    * To find all rows that intersect the vertical interval aY/aYMost, call
>    * GetFirstRowContaining(aY, &overflowAbove), and then iterate through all
>    * rows until reaching a row where row->GetRect().y - overflowAbove >= aYMost.
>    * That row and all subsequent rows cannot intersect the interval.
>    */
>-  nsIFrame* GetFirstRowContaining(nscoord aY, nscoord* aOverflowAbove);
>+  nsIFrame* GetFirstRowContaining(nscoord aBCoord, nscoord* aOverflowAbove);

Please revert this line, beause:
 - This patch isn't actually changing the function's impl, in the .cpp file. (There, we still call this arg "aY")
 - The sole caller of this method still passes in a physical y-value. (That one caller is in TableBackgroundPainter::PaintRowGroup(), and it passes in "mDirtyRect.y - mRenderPt.y").

(Please make sure there's a followup bug filed on logicalizing this function, too.)
Attachment #8624178 - Flags: review?(dholbert) → review+
Comment on attachment 8624786 [details] [diff] [review]
patch 2 - Convert nsTableFrame::SetColumnDimensions to work with logical-order iteration over the table, so that column coordinates and collapsed borders are correct

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

::: layout/tables/nsTableFrame.cpp
@@ +1424,5 @@
> +                                  const LogicalMargin& aBorderPadding,
> +                                  nscoord aContainerWidth)
> +{
> +  const nscoord colBSize = aBSize - aBorderPadding.BStartEnd(aWM) +
> +                           GetRowSpacing(-1) + GetRowSpacing(GetRowCount());

Oops, this needs parens (as in comment 14), or else the + signs should be changed to - signs. Tryserver helpfully pointed out that I missed this, and thereby broke a couple of tests. Fixed locally in preparation for landing.
Main patch, folding in patch 1a and updated to address review comments, in preparation for landing. Carrying over r=dholbert.
Attachment #8624178 - Attachment is obsolete: true
(In reply to Daniel Holbert [:dholbert] from comment #21)
> >+    LogicalPoint origKidNormalPosition =
> >+      kidFrame->GetLogicalNormalPosition(wm, containerWidth);
> >+    MOZ_ASSERT(origKidNormalPosition.B(wm) == 0 || wm.IsVerticalRL());
> 
> This assertion (particularly the new wm.IsVerticalRL() special-case) is
> confusing.
> 
> I think the assertion is basically saying:
>  - All cells' no-relative-positioning position should be snapped to the
> row's bstart edge.
> except I don't understand why wm.IsVerticalRL() needs an exception.

I added a comment here, and removed the IsVerticalRL() exception; it was there because when I was actually trying vertical modes, this blocked any further testing. I think it does indicate a bug (likely a bad containerWidth issue at some level), but wanted to be able to get past this during ongoing patch development. I may still want that exception in the code temporarily, but if so will reintroduce it when needed.
Comment on attachment 8624757 [details] [diff] [review]
patch 1a - Address review issues from comment 15

Marking "patch 1a" as obsolete now that it's folded into the main patch.
Attachment #8624757 - Attachment is obsolete: true
(In reply to Daniel Holbert [:dholbert] from comment #23)
> > nscoord
> >-nsTableRowGroupFrame::CollapseRowGroupIfNecessary(nscoord aYTotalOffset,
> >-                                                  nscoord aWidth)
> >+nsTableRowGroupFrame::CollapseRowGroupIfNecessary(nscoord aBTotalOffset,
> >+                                                  nscoord aISize)
> > {
> >+  WritingMode wm = GetWritingMode(); // XXX pass from caller
> 
> Did you meant to leave this XXX comment in?  (Looks like there's only one
> caller -- nsTableFrame::AdjustForCollapsingRowsCols -- and it already has
> "aWM", which it can pass to us. So, shouln't be too hard to address it here.)

This is fixed in the patches for nsTableFrame (bug 1174711), iirc, but to avoid bit-rotting those I've left it as is for now.
Blocks: 1176522
(In reply to Daniel Holbert [:dholbert] from comment #23)
> >   nsTableFrame* tableFrame = GetTableFrame();
> >+  nscoord containerWidth = tableFrame->GetRect().width;
> 
> I think this containerWidth value is stale. This function
> (CollapseRowGroupIfNecessary) is called while the tableFrame is being
> reflowed, so it's about to get a new & different width (which is actually
> currently being adjusted).
> 
> Maybe file a followup to cover this and drop an XXX comment here pointing to
> that followup? I'm not sure what the right containerWidth to use here is.
> Our caller (AdjustForCollapsingRowsCols) is in the process of updating the
> table's aDesiredSize, so it could pass us aDesiredSize.Width() as *some*
> relatively up-to-date containerWidth, but that's not really right, because
> it could still end up changing after that.

Bug 1176522.

> Please revert this line, beause:
>  - This patch isn't actually changing the function's impl, in the .cpp file.
> (There, we still call this arg "aY")
>  - The sole caller of this method still passes in a physical y-value. (That
> one caller is in TableBackgroundPainter::PaintRowGroup(), and it passes in
> "mDirtyRect.y - mRenderPt.y").
> 
> (Please make sure there's a followup bug filed on logicalizing this
> function, too.)

Bug 1176524.
(In reply to Jonathan Kew (:jfkthame) from comment #30)
> This is fixed in the patches for nsTableFrame (bug 1174711), iirc, but to
> avoid bit-rotting those I've left it as is for now.

Indeed, "Patch 2" over there. Thanks!
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b60de195ad59 for assertions (see commit message of backout).
Flags: needinfo?(jfkthame)
... actually, it hit debug reftests on all platforms.
Ah, drat... during the final adjustments, I forgot we already had a test in the tree that would hit that assertion (because of how <marquee> is implemented). I'll disable that part of the test for now, with a comment pointing to bug 1132308.
Flags: needinfo?(jfkthame)
https://hg.mozilla.org/mozilla-central/rev/e7d39a6671ea
https://hg.mozilla.org/mozilla-central/rev/056a7557fad6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: