349246 - [reflow branch] Gmail compose textarea has a too small height

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

In the reflow branch, the gmail compose textarea has a too small height compared to the normal builds.
I'll attach a testcase that shows the bug.
It uses the document.body.offsetHeight hack to trigger the bug.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Smaller testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

You can also get this by removing the <script> and just resizing the window after the load is done.  I don't see a special height reflow happening in that case, so we never hit this code in nsTableCellFrame::Reflow:

  if (aReflowState.mFlags.mSpecialHeightReflow) {
    ((nsHTMLReflowState&)aReflowState).mComputedHeight = mRect.height - topInset - bottomInset;

So we have an auto computed height when we reflow the block....

Comment 4

[Boris Zbarsky [:bzbarsky]]

We do call nsTableFrame::RequestSpecialHeightReflow in this case.  Not sure why we don't get one yet.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So in nsTableRowGroupFrame::ReflowChildren we end up with reflowAllKids false, so we skip reflowing the table row....

Comment 6

[Boris Zbarsky [:bzbarsky]]

    if ((reflowAllKids ||
         (kidFrame->GetStateBits() &
          (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) &&
        !(aReflowState.reflowState.mFlags.mSpecialHeightReflow &&
          !isPaginated &&
          !((nsTableRowFrame*)kidFrame)->NeedSpecialReflow())) {

That seems a little odd to me.  Should that '&&' between the two parts of the condition be an '||'?  It seems to me like we should reflow any kid that needs a special reflow...

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It's pretty much equivalent to what the old code did, assuming I didn't make a logic error:

-    PRBool doReflowChild = PR_TRUE;
-    if (aDirtyOnly && ((kidFrame->GetStateBits() & NS_FRAME_IS_DIRTY) == 0)) {
-      doReflowChild = PR_FALSE;
-    }
-    nsIAtom* kidType = kidFrame->GetType();
-    if (aReflowState.reflowState.mFlags.mSpecialHeightReflow) {
-      if (!isPaginated && (nsLayoutAtoms::tableRowFrame == kidType &&
-                           !((nsTableRowFrame*)kidFrame)->NeedSpecialReflow()))-        doReflowChild = PR_FALSE;
-      }
     }


(That said, I wonder if I can get rid of a lot of this special height reflow stuff by reusing my new height-dependent reflow optimizations.)

Comment 8

[Martijn Wargers (dead)]

I remember I had an in-between testcase that also showed the bug on normal trunk builds, so I guess the bug exists there too in some way. But on normal trunk builds, the bug doesn't manifest itself on gmail compose.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The logic does look the same if reflowAllKids == !aDirtyOnly...  But conceptually it still looks wrong...

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

For what it's worth, the following change:

-    if ((reflowAllKids ||
-         (kidFrame->GetStateBits() &
-          (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) &&
-        !(aReflowState.reflowState.mFlags.mSpecialHeightReflow &&
-          !isPaginated &&
-          !((nsTableRowFrame*)kidFrame)->NeedSpecialReflow())) {
+    if (reflowAllKids ||
+        (kidFrame->GetStateBits() &
+         (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) ||
+        (aReflowState.reflowState.mFlags.mSpecialHeightReflow &&
+         ((nsTableRowFrame*)kidFrame)->NeedSpecialReflow())) {

does NOT fix this bug.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I needed changes in a bunch of places, but it's now fixed.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Er, the smaller testcase is, but I need more work on the other one...

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

OK, now I fixed the first testcase with this rather ugly change to nsHTMLReflowState::InitResizeFlags:

-  if (((mStylePosition->mHeight.GetUnit() == eStyleUnit_Percent &&
-        mComputedHeight != NS_AUTOHEIGHT) ||
+  // It would be nice to check that |mComputedHeight != NS_AUTOHEIGHT|
+  // &&ed with the percentage height check.  However, this doesn't get
+  // along with table special height reflows, since a special height
+  // reflow (a quirk that makes such percentage heights work on children
+  // of table cells) can cause not just a single percentage height to
+  // become fixed, but an entire descendant chain of percentage heights
+  // to become fixed.
+  if ((mStylePosition->mHeight.GetUnit() == eStyleUnit_Percent ||

However, gmail itself is still broken.

Martijn, any chance you could come up with a reduced testcase for the remaining problem?

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

A number of checkins later, actually fixed.

Comment 15

[Martijn Wargers (dead)]

Sorry for not providing a testcase.