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

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: dbaron)

Tracking

({testcase})

Trunk
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 234507 [details]
testcase
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....
We do call nsTableFrame::RequestSpecialHeightReflow in this case.  Not sure why we don't get one yet.
So in nsTableRowGroupFrame::ReflowChildren we end up with reflowAllKids false, so we skip reflowing the table row....
    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...
(Assignee)

Comment 7

12 years ago
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.)
(Reporter)

Comment 8

12 years ago
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.
The logic does look the same if reflowAllKids == !aDirtyOnly...  But conceptually it still looks wrong...
(Assignee)

Comment 10

12 years ago
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.
(Assignee)

Comment 11

12 years ago
I needed changes in a bunch of places, but it's now fixed.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

12 years ago
Er, the smaller testcase is, but I need more work on the other one...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

12 years ago
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?
(Assignee)

Comment 14

12 years ago
A number of checkins later, actually fixed.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

12 years ago
Sorry for not providing a testcase.
You need to log in before you can comment on or make changes to this bug.