393970 - Grid columns don't line up if one <rows> block is scrollable

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Wladimir Palant]

Attachment: Testcase

The problem occurs when using <grid> with two nested <rows> blocks - first for header, second for list body. Only the second <rows> block is scrollable. In Firefox 2.0 the columns are still lined up (last cell of the header is made larger by the width of the scroll bar), on trunk however the cells of the header are shifted (proportions are simply kept, and header has more space resulting in a shift).

Comment 1

[Wladimir Palant]

This regressed between 2007-01-31-04 and 2007-02-01-04. My bet is on bug 243370, no other patch touched grid on this day. Cc'ing Andreas Lange and roc to comment.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could be, but I don't see the bug looking at attachment #253459 [details] [diff] [review]. This needs to be debugged.

Comment 3

[Wladimir Palant]

I can confirm that patch from attachment #253459 [details] [diff] [review] caused the issue - I checked out code from 2007-02-01 and the issue occurs there, backing out this patch fixed it. I don't see any bugs in this patch either, so I will try to find out which part of the patch causes the regression.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks!

Comment 5

[Wladimir Palant]

I am still recompiling but the bug seems to be caused by this change in nsGridRowLeafLayout.cpp:

      // go up the parent chain looking for scrollframes
-     PRBool isHorizontal = PR_FALSE;
-     aBox->GetOrientation(isHorizontal);
-
      nsIBox* scrollbox = nsnull;
      aBox->GetParentBox(&aBox);
      scrollbox = nsGrid::GetScrollBox(aBox);
        
        nsCOMPtr<nsIScrollableFrame> scrollable = do_QueryInterface(scrollbox);
        if (scrollable) {
           nsMargin scrollbarSizes = scrollable->GetActualScrollbarSizes();
 
           nsRect ourRect(scrollbox->GetRect());
           nsMargin padding(0,0,0,0);
           scrollbox->GetBorderAndPadding(padding);
           ourRect.Deflate(padding);
           scrollbox->GetInset(padding);
           ourRect.Deflate(padding);
 
           nscoord diff;
-          if (isHorizontal) {
+          if (aBox->IsHorizontal()) {
             diff = scrollbarSizes.left + scrollbarSizes.right;
           } else {
             diff = scrollbarSizes.top + scrollbarSizes.bottom;
           }

By the time we call aBox->IsHorizontal() we have already overwritten the value of aBox.

Comment 6

[Wladimir Palant]

Attachment: Patch

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 278874 [details] [diff] [review]
Patch

bingo! Thanks a lot!

Comment 8

[Wladimir Palant]

Attachment: Mochitest testcase

The testcase shows that we still have issues here - columns don't line up if we put another <rows> block inside a <rows> block with vertical overflow. This bug is not a regression however, same happens in Firefox 2.0 as well.

Comment 9

[Wladimir Palant]

Attachment: More extensive testcase

Comment 10

[Wladimir Palant]

Attachment: Patch v2

The new patch now fixes the second problem as well - GetScrollBox() would only go up to the next grid part, we should check all parents however. I also removed some unused code (ourRect) and added a return at the end of |if (diff > 0)| - as far as I can tell we don't need to call nsSprocketLayout::ComputeChildSizes() again (?).

Unfortunately, I know very little about the layout code, so maybe I am doing something wrong here. I think we should be stopping walking the parent chain at the grid, but so far I could only come up with:

      // if we are at our grid - we are done
      const nsStyleDisplay* display = scrollbox->GetStyleContext()->GetStyleDisplay();
      if (display->mDisplay == NS_STYLE_DISPLAY_INLINE_GRID ||
          display->mDisplay == NS_STYLE_DISPLAY_GRID)
      {
        break;
      }

However, I guess using display property here would be wrong, and for some reason I couldn't make this patch fail without this check.

I fixed the alignment of rows, it was to difficult to edit this function. I will attach a -w patch as well.

Comment 11

[Wladimir Palant]

Attachment: Patch v2 with -w

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can  you check in the first patch, and then we'll do the second issue as a separate patch? Thanks!

Comment 13

[Reed Loden [:reed]]

First patch checked in. Leaving open for second patch.

Checking in layout/xul/base/src/grid/nsGridRowLeafLayout.cpp;/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp,v  <--  nsGridRowLeafLayout.cpp
new revision: 1.19; previous revision: 1.18
done

Comment 14

[[e-mail]]

(In reply to comment #10)
>I think we should be stopping walking the parent chain at the grid
Can you just keep calling GetParentGridPart until it returns null?

Comment 15

[Wladimir Palant]

(In reply to comment #14)
> Can you just keep calling GetParentGridPart until it returns null?

Not sure about that - can we have scrollable boxes inside a grid that are not grid parts?

Comment 16

[Wladimir Palant]

Attachment: Patch v3

Attaching updated patch, it no longer contains the regression fix that was already checked in. This patch also implements Neil's suggestion.

Comment 17

[Wladimir Palant]

Attachment: Patch v3 with -w

Comment 18

[[e-mail]]

Comment on attachment 280449 [details] [diff] [review]
Patch v3 with -w

>        nsCOMPtr<nsIScrollableFrame> scrollable = do_QueryInterface(scrollbox);
I didn't think frames were refcounted - you're going to get blamed for reindenting buggy code without fixing it ;-)

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> (From update of attachment 280449 [details] [diff] [review])
> >        nsCOMPtr<nsIScrollableFrame> scrollable = do_QueryInterface(scrollbox);
> I didn't think frames were refcounted - you're going to get blamed for
> reindenting buggy code without fixing it ;-)

Wladimir / Robert: Do you want this issue fixed or have the patch checked-in as-is?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It doesn't need to be fixed here.

Comment 21

[Reed Loden [:reed]]

Checking in layout/xul/base/Makefile.in;
/cvsroot/mozilla/layout/xul/base/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
Checking in layout/xul/base/src/grid/nsGridRowLeafLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp,v  <--  nsGridRowLeafLayout.cpp
new revision: 1.20; previous revision: 1.19
done
RCS file: /cvsroot/mozilla/layout/xul/base/test/Makefile.in,v
done
Checking in layout/xul/base/test/Makefile.in;
/cvsroot/mozilla/layout/xul/base/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/xul/base/test/test_bug393970.xul,v
done
Checking in layout/xul/base/test/test_bug393970.xul;
/cvsroot/mozilla/layout/xul/base/test/test_bug393970.xul,v  <--  test_bug393970.xul
initial revision: 1.1
done

Comment 22

[Reed Loden [:reed]]

Four of the tests added by this bug failed, so I had to backout the patch:

*** 36036 ERROR FAIL | Cells (1,2) and (2,2) line up horizontally (with overflow-x: hidden; overflow-y: scroll;) | got 435, expected 438 | /tests/layout/xul/base/test/test_bug393970.xul
*** 36038 ERROR FAIL | Cells (1,3) and (2,3) line up horizontally (with overflow-x: hidden; overflow-y: scroll;) | got 468, expected 471 | /tests/layout/xul/base/test/test_bug393970.xul
*** 36048 ERROR FAIL | Cells (1,2) and (2,2) line up horizontally (with overflow-x: scroll; overflow-y: scroll;) | got 435, expected 438 | /tests/layout/xul/base/test/test_bug393970.xul
*** 36050 ERROR FAIL | Cells (1,3) and (2,3) line up horizontally (with overflow-x: scroll; overflow-y: scroll;) | got 468, expected 471 | /tests/layout/xul/base/test/test_bug393970.xul

Comment 23

[Wladimir Palant]

For some reason, the testcase works correctly on its own, but it doesn't like being loaded into an iframe.

Comment 24

[Wladimir Palant]

Attachment: Patch v4

Removed bitrot and changed width of the <grid> element in testcase to be 1000px - columns will be shifted if there is not enough space to display them properly, but that's a different issue (and maybe not even an issue).

Requesting approval for 1.9 - TomTom HOME 2.2 was released with that change several months ago, a central UI element uses a grid with fixed header and scrollable body, no issues reported.

Comment 25

[Wladimir Palant]

Attachment: Patch v4 with -w

Comment 26

[Damon Sicore (:damons)]

Seems to me there's a decent chance this could regress something since we're updating a bitrotted patch, what's the risk here?  I'm hesitant to take this at this point without serious justification. 

Comment 27

[Wladimir Palant]

The bitrot was in the whitespace changes only. And this regression does make the <grid> element much less useful than it should be.

This patch only affects the size calculation for grids with scrollable parts - and that is already broken on trunk. I don't see how we can do worse than we are doing now.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314308 [details] [diff] [review]
Patch v4

a1.9=beltzner

Comment 29

[:Gavin Sharp [email: [e-mail]]]

mozilla/layout/xul/base/test/Makefile.in 	1.3
mozilla/layout/xul/base/test/test_bug393970.xul 	1.3
mozilla/layout/xul/base/Makefile.in 	1.8
mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp 	1.25

Comment 30

[:Gavin Sharp [email: [e-mail]]]

The test is failing on the linux tinderbox. I disabled it temporarily and filed bug 428226.