1177600 - fix table row positions in vertical-rl writing mode when row/rowgroup block size is modified

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jonathan Kew [:jfkthame]]

This is the bug where dynamically modifying cell contents/sizes in a vertical-rl table causes row positions to go haywire, due to incorrect use of containerWidths. Fixing this will resolve the Android failures that occurred when the tests in bug 1077521 were landed.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Properly adjust table row and cell positions when their containing block-size changes in vertical-rl writing mode

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8626390 [details] [diff] [review]
Properly adjust table row and cell positions when their containing block-size changes in vertical-rl writing mode

Haven't gotten all the way through this yet; here's what I've got so far:

>+++ b/layout/tables/nsTableFrame.cpp
>@@ -1823,16 +1823,17 @@ nsTableFrame::Reflow(nsPresContext*     
> 
>   bool haveDesiredBSize = false;
>   SetHaveReflowedColGroups(false);
> 
>   // Reflow the entire table (pass 2 and possibly pass 3). This phase is necessary during a
>   // constrained initial reflow and other reflows which require either a strategy init or balance.
>   // This isn't done during an unconstrained reflow, because it will occur later when the parent
>   // reflows with a constrained isize.
>+  bool fixupKidPositions = false;
[...]
>+    // If ComputedWidth is unconstrained, we may need to fix child positions
>+    // later (in vertical-rl mode) due to use of 0 as a fake containerWidth
>+    // during ReflowChildren.
>+    fixupKidPositions = aReflowState.ComputedWidth() == NS_UNCONSTRAINEDSIZE;
[...]
>+  if (fixupKidPositions && wm.IsVerticalRL()) {

I think this would be simpler/more-grokable if we folded the wm.IsVerticalRL() check into your fixupKidPositions assignment.

So e.g.:
  fixupKidPositions = wm.IsVerticalRL() &&
    aReflowState.ComputedWidth() == NS_UNCONSTRAINEDSIZE;
  [...]
  if (fixupKidPositions) {


>   if (containerWidth == NS_UNCONSTRAINEDSIZE) {
[...]
>+    // We won't know the containerWidth until we've reflowed our contents,
>+    // so use zero for now; in vertical-rl mode, this will mean the children
>+    // are misplaced in the block-direction, and will need to be moved
>+    // rightwards by the true containerWidth once we know it.
>+    containerWidth = 0;
>   } else {
>     containerWidth +=
>       aReflowState.reflowState.ComputedPhysicalBorderPadding().LeftRight();
>   }

(looks like this section will benefit from bug 1177614)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8626390 [details] [diff] [review]
Properly adjust table row and cell positions when their containing block-size changes in vertical-rl writing mode

>+++ b/layout/tables/nsTableRowFrame.cpp
>+      // Resize the cell's bsize if it has changed.
>       LogicalSize cellSize = cellFrame->GetLogicalSize(wm);
>-      nsRect cellVisualOverflow = cellFrame->GetVisualOverflowRect();
>-      if (cellSize.BSize(wm) != cellBSize) {
>+      if (cellSize.BSize(wm) != cellBSize || wm.IsVerticalRL()) {

Can you expand the comment here to cover (or at least hint at) the IsVerticalRL check here? That part of the "if" check doesn't seem to be related at all to the comment, which makes the comment confusing. Alternately, the logic might want to be restructured, per below... (which obsoletes this review-nit here)

>+        // For vertical-rl mode, we also need to reset the cell's block-coord
>+        // to account for the potential change in containerWidth.
>+        // The difference (if any) between oldPos and oldNormalPos reflects
>+        // relative positioning that may have been applied to the cell,
>+        // and which we need to incorporate when resetting the position.
>+        LogicalPoint oldPos = cellFrame->GetLogicalPosition(wm, containerWidth);
>+        LogicalPoint oldNormalPos =
>+          cellFrame->GetLogicalNormalPosition(wm, containerWidth);
>+        LogicalPoint newPos(wm, oldPos.I(wm), oldPos.B(wm) - oldNormalPos.B(wm));
>+
>         cellSize.BSize(wm) = cellBSize;
>-        nsRect cellOldRect = cellFrame->GetRect();
>+
>+        if (oldPos != newPos) {
>+          cellFrame->SetPosition(wm, newPos, containerWidth);
>+          nsTableFrame::RePositionViews(cellFrame);
>+        }
>+

Maybe put this whole chunk inside a dedicated IsVerticalRL() check?  We only need this code in vertical-rl mode, right?

(This is inside an existing "cellSize.BSize(wm) != cellBSize || wm.IsVerticalRL()" check, so we'll be checking IsVerticalRL twice, but that's OK. It saves us from having to run all of this code in other writing-modes.)

Also: can we use nsIFrame::MovePositionBy to update the frame position here? That handles relative positioning pretty nicely. (I don't 100% follow the position modification steps we're doing right now... "newPos" seems more like it's our relative-positioning offset, or something? [difference between old actual position & old normal position])  MovePositionBy may make this code more graceful/understandable.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8626390 [details] [diff] [review]
> Properly adjust table row and cell positions when their containing
> block-size changes in vertical-rl writing mode
> 
> >+++ b/layout/tables/nsTableRowFrame.cpp
> >+      // Resize the cell's bsize if it has changed.
> >       LogicalSize cellSize = cellFrame->GetLogicalSize(wm);
> >-      nsRect cellVisualOverflow = cellFrame->GetVisualOverflowRect();
> >-      if (cellSize.BSize(wm) != cellBSize) {
> >+      if (cellSize.BSize(wm) != cellBSize || wm.IsVerticalRL()) {
> 
> Can you expand the comment here to cover (or at least hint at) the
> IsVerticalRL check here? That part of the "if" check doesn't seem to be
> related at all to the comment, which makes the comment confusing.
> Alternately, the logic might want to be restructured, per below... (which
> obsoletes this review-nit here)
> 
> >+        // For vertical-rl mode, we also need to reset the cell's block-coord
> >+        // to account for the potential change in containerWidth.
> >+        // The difference (if any) between oldPos and oldNormalPos reflects
> >+        // relative positioning that may have been applied to the cell,
> >+        // and which we need to incorporate when resetting the position.
> >+        LogicalPoint oldPos = cellFrame->GetLogicalPosition(wm, containerWidth);
> >+        LogicalPoint oldNormalPos =
> >+          cellFrame->GetLogicalNormalPosition(wm, containerWidth);
> >+        LogicalPoint newPos(wm, oldPos.I(wm), oldPos.B(wm) - oldNormalPos.B(wm));
> >+
> >         cellSize.BSize(wm) = cellBSize;
> >-        nsRect cellOldRect = cellFrame->GetRect();
> >+
> >+        if (oldPos != newPos) {
> >+          cellFrame->SetPosition(wm, newPos, containerWidth);
> >+          nsTableFrame::RePositionViews(cellFrame);
> >+        }
> >+
> 
> Maybe put this whole chunk inside a dedicated IsVerticalRL() check?  We only
> need this code in vertical-rl mode, right?
> 
> (This is inside an existing "cellSize.BSize(wm) != cellBSize ||
> wm.IsVerticalRL()" check, so we'll be checking IsVerticalRL twice, but
> that's OK. It saves us from having to run all of this code in other
> writing-modes.)
> 
> Also: can we use nsIFrame::MovePositionBy to update the frame position here?

I couldn't get that to work, because that would require knowing the *change* that has happened to the rowGroup's BSize, and by the time this gets called, the old value is long gone.

> That handles relative positioning pretty nicely. (I don't 100% follow the
> position modification steps we're doing right now... "newPos" seems more
> like it's our relative-positioning offset, or something? [difference between
> old actual position & old normal position])  MovePositionBy may make this
> code more graceful/understandable.

The thinking here is that the difference between old actual and old normal positions is the result of any relative-positioning offset that has been applied, and which we should preserve here. The new inline position will be the same as the old actual inline position, as we're only dealing with a block-dir change here; and the new block position will be zero plus any relative-pos offset that had been applied.

I'll try to write some better comments...

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Properly adjust table row and cell positions when their containing block-size changes in vertical-rl writing mode

This may be a bit easier to follow (I hope)...

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8626812 [details] [diff] [review]
Properly adjust table row and cell positions when their containing block-size changes in vertical-rl writing mode

Looks good, thanks!

(I guess this can be marked in-testsuite, without needing its own reftest, given that this fixes an android test-failure per comment 0?)

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> (I guess this can be marked in-testsuite, without needing its own reftest,
> given that this fixes an android test-failure per comment 0?)

Yes; I'll re-land the tests from bug 1077521, which bounced last time because of this issue. It is also necessary for other table tests that will be landing shortly.

Comment 8

[Jonathan Kew [:jfkthame]]

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

Comment 9

[Carsten Book [:Tomcat]]

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