Closed
Bug 402629
Opened 17 years ago
Closed 16 years ago
Table doesn't shrink on browser resize, with nested table/div/table and 100% height on everything
Categories
(Core :: Layout: Tables, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: dholbert, Assigned: dholbert)
Details
(Keywords: regression, testcase)
Attachments
(9 files, 1 obsolete file)
439 bytes,
text/html
|
Details | |
505 bytes,
text/html
|
Details | |
539 bytes,
text/html
|
Details | |
972 bytes,
text/html
|
Details | |
898 bytes,
text/html
|
Details | |
728 bytes,
text/html
|
Details | |
761 bytes,
text/html
|
Details | |
3.95 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
10.93 KB,
patch
|
Details | Diff | Splinter Review |
I'm filing this new bug for the testcases that robinpelgrim@zonnet.nl recently posted on bug 380227, so as not to get that bug too complicated, because it's closed. (and this seems like a slightly different issue) The testcases posted on bug 380227 were: attachment 287438 [details] attachment 287439 [details] attachment 287441 [details] I'm attaching a reduced version of attachment 287438 [details]. This bug is specific to standards mode. (Removing the DOCTYPE declaration fixes it.) Also, the 100%-height on the <html> element is required to trigger the bug.
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 1•17 years ago
|
||
In this testcase, the table grows indefinitely on window-resizes. (even when you shrink the window)
Assignee | ||
Comment 2•17 years ago
|
||
Reduced testcase, based on attachment 287439 [details].
Experiment A:
1. Load testcase 3
2. Shrink window vertically, to be smaller than the table
Results: Table doesn't shrink at first, but then "snaps" smaller when window gets smaller than it.
Experiment B:
1. Load testcase 3
2. Shrink window a bit vertically
3. Resize window horizontally
Results: Window doesn't shrink at first, but then "snaps" smaller when you do the horizontal resize.
For both experiments, the "snap" triggers this warning:
WARNING: Strange content ... we can't find logically consistent scrollbar settings: file /scratch/work/builds/trunk.07-11-05.10-25/mozilla/layout/generic/nsGfxScrollFrame.cpp, line 599
Assignee | ||
Updated•17 years ago
|
Attachment #287470 -
Attachment description: testcase 1 → testcase 1 (table never shrinks)
Assignee | ||
Updated•17 years ago
|
Keywords: regression,
testcase
This worked in FF2, I presume?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee | ||
Comment 4•17 years ago
|
||
Yup, all three testcases work in FF2.
Comment 5•17 years ago
|
||
Here another test case where 100% div in table doesn't behave properly. (No vertical scrollbar appearing in div when parentNode too small.) (This is needed for having a page-header and page-footer and let the body stretch/shrink with the window size...)
Comment 6•17 years ago
|
||
(adjusted previous testcase (for better understanding))
Attachment #288129 -
Attachment is obsolete: true
Comment 7•17 years ago
|
||
textarea seems to behave correctly when shrinking the main window...
Assignee | ||
Comment 8•17 years ago
|
||
From debugging through Testcase 3, here's one bit of info. (see Comment #2, "Experiment A") The reason we do grow but don't shrink the table for vertical window-resizes traces back to these lines in nsTableRowGroupFrame.cpp#745: 745 nscoord extra = pctHeightBasis - heightOfRows; 746 for (rowFrame = startRowFrame, rowIndex = 0; rowFrame && (extra > 0); rowFrame = rowFrame->GetNextRow(), rowIndex++) { ... // Distribute extra height to rows 754 } Link: http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.cpp#745 When we increase height by one notch, extra is +300. When we decrease height by one notch, extra is -300. So when we reduce the height, we don't enter the distribute-extra-height loop, because the "extra > 0" condition at line 746 is false. That doesn't help too much though... we're probably not setting a dirty flag somewhere, when we should be. Basically, we probably shouldn't get to a point where we're reflowing rows using their old heights, because the height needs to be reduced to less than that.
This sounds awfully familiar... like bug 380227?
Assignee | ||
Comment 10•17 years ago
|
||
Yup, very similar. I think we basically need to add a call to SetGeometryDirty, as in that bug. Unconditionally calling SetGeometryDirty at the beginning of nsTableFrame::Reflow fixes this. I'm gonna figure out specifically where / when the call is needed.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 11•17 years ago
|
||
hmm... looks like mVResize isn't turned on in the first reflow of the inner table. Specifically, we hit this code with aReflowState.mFlags.mVResize = 0, and we skip over SetGeometryDirty() as a result. 1850 if (aReflowState.ComputedHeight() != NS_UNCONSTRAINEDSIZE || 1851 // Also check mVResize, to handle the first Reflow preceding a 1852 // special height Reflow, when we've already had a special height 1853 // Reflow (where mComputedHeight would not be 1854 // NS_UNCONSTRAINEDSIZE, but without a style change in between). 1855 aReflowState.mFlags.mVResize) { ... 1863 SetGeometryDirty(); 1864 } http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableFrame.cpp#1855 But this is definitely a vertical resize. So, mVResize isn't getting passed down for some reason. Stepping up the callstack to nsTableOuterFrame::OuterReflowChild, we have aOuterRS.mFlags.mVResize == 1 but childRS.mFlags.mVResize == 0 Investigating where that discrepancy comes from...
Assignee | ||
Comment 12•17 years ago
|
||
Assignee | ||
Comment 13•17 years ago
|
||
So, looking at testcase6, it looks like the problem is with initializing the inner table's nsHTMLReflowState's mVResize flag, in nsHTMLReflowState::InitResizeFlags, at http://mxr.mozilla.org/seamonkey/source/layout/generic/nsHTMLReflowState.cpp#388 Right now, we're hitting this case for testcase6: 414 mFlags.mVResize = mFlags.mHResize || NS_SUBTREE_DIRTY(frame); which ends up being "false". Here's how we can change testcase6 to make it work, and why: - If we take out the div that wraps the inner table, then we hit: 405 } else if (mCBReflowState && !frame->IsContainingBlock()) { ... 408 mFlags.mVResize = mCBReflowState->mFlags.mVResize; - If we remove the DOCTYPE declaration and go into quirks mode, then we hit 410 if (eCompatibility_NavQuirks == aPresContext->CompatibilityMode() && 411 mCBReflowState) { 412 mFlags.mVResize = mCBReflowState->mFlags.mVResize; Both of these result in setting mFlags.mVResize to true. So, I think we need to extend the case from 405-408 to include frames with IsContainingBlock IFF they have percent-height. Something like that.
Assignee | ||
Comment 14•17 years ago
|
||
(In reply to comment #13) > - If we take out the div that wraps the inner table, then we hit: > 405 } else if (mCBReflowState && !frame->IsContainingBlock()) { > ... > 408 mFlags.mVResize = mCBReflowState->mFlags.mVResize; Oops, I lied -- we don't hit that clause in the no-div reference case. We hit the same clause as in the testcase, line 414, and we set mFlags.mVResize to false. BUT then we turn on mFlags.mVResize here: 431 // If we're the child of a table cell that performs special height 432 // reflows and we could be the child that requires them, always set 433 // the vertical resize in case this is the first pass before the 434 // special height reflow. 435 if (!mFlags.mVResize && mCBReflowState && 436 IS_TABLE_CELL(mCBReflowState->frame->GetType()) && 437 dependsOnCBHeight) 438 mFlags.mVResize = PR_TRUE; Now, in testcase6, with the div wrapping the inner table, we don't turn mVResize on there because the containing block isn't a table cell -- it's a div. We probably want to generalize the IS_TABLE_CELL there to something like HAS_ANCESTOR_CELL or DEPENDS_ON_ANCESTOR_CELL_HEIGHT.
Priority: P3 → P2
Assignee | ||
Comment 15•16 years ago
|
||
This is the like testcase 6, but with multiple nested divs in a row.
Assignee | ||
Comment 16•16 years ago
|
||
(In reply to comment #14) > We probably want to generalize the IS_TABLE_CELL there to something like > HAS_ANCESTOR_CELL or DEPENDS_ON_ANCESTOR_CELL_HEIGHT. This patch implements that strategy, and it fixes testcase 1,2,3,6,and 7. I'm pretty sure Testcase 4 with the missing scrollbar (attachment 288704 [details]) is a different issue.
Attachment #300747 -
Flags: superreview?(dbaron)
Attachment #300747 -
Flags: review?(dbaron)
Assignee | ||
Updated•16 years ago
|
Attachment #289709 -
Attachment description: test case 5 scrollbars do appear for textarea → test case 5 (reference -- scrollbars do appear for textarea)
Assignee | ||
Updated•16 years ago
|
Attachment #288704 -
Attachment description: Test case 4 missing scrollbar → Test case 4 (missing scrollbar)
Assignee | ||
Comment 17•16 years ago
|
||
I filed Bug 415172 for testcases 4 and 5. They're unrelated to the issue on this bug, AFAICT.
Status: NEW → ASSIGNED
Comment on attachment 300747 [details] [diff] [review] patch v1 Looks good. r+sr=dbaron. Sorry for the delay getting to this.
Attachment #300747 -
Flags: superreview?(dbaron)
Attachment #300747 -
Flags: superreview+
Attachment #300747 -
Flags: review?(dbaron)
Attachment #300747 -
Flags: review+
Assignee | ||
Comment 19•16 years ago
|
||
Here's the patch as-landed -- I just added some reftests based on testcases 2, 6, and 7. Checking in generic/nsHTMLReflowState.cpp; /cvsroot/mozilla/layout/generic/nsHTMLReflowState.cpp,v <-- nsHTMLReflowState.cpp new revision: 1.291; previous revision: 1.290 done Checking in generic/nsHTMLReflowState.h; /cvsroot/mozilla/layout/generic/nsHTMLReflowState.h,v <-- nsHTMLReflowState.h new revision: 3.74; previous revision: 3.73 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-1-iframe.html,v done Checking in reftests/bugs/402629-1-iframe.html; /cvsroot/mozilla/layout/reftests/bugs/402629-1-iframe.html,v <-- 402629-1-iframe.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-1-ref.html,v done Checking in reftests/bugs/402629-1-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402629-1-ref.html,v <-- 402629-1-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-1.html,v done Checking in reftests/bugs/402629-1.html; /cvsroot/mozilla/layout/reftests/bugs/402629-1.html,v <-- 402629-1.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-2-iframe.html,v done Checking in reftests/bugs/402629-2-iframe.html; /cvsroot/mozilla/layout/reftests/bugs/402629-2-iframe.html,v <-- 402629-2-iframe.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-2-ref.html,v done Checking in reftests/bugs/402629-2-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402629-2-ref.html,v <-- 402629-2-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-2.html,v done Checking in reftests/bugs/402629-2.html; /cvsroot/mozilla/layout/reftests/bugs/402629-2.html,v <-- 402629-2.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-3-iframe.html,v done Checking in reftests/bugs/402629-3-iframe.html; /cvsroot/mozilla/layout/reftests/bugs/402629-3-iframe.html,v <-- 402629-3-iframe.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-3-ref.html,v done Checking in reftests/bugs/402629-3-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402629-3-ref.html,v <-- 402629-3-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402629-3.html,v done Checking in reftests/bugs/402629-3.html; /cvsroot/mozilla/layout/reftests/bugs/402629-3.html,v <-- 402629-3.html initial revision: 1.1 done Checking in reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.361; previous revision: 1.360 done
Assignee | ||
Comment 20•16 years ago
|
||
Patch landed --> closing as fixed.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9beta4
You need to log in
before you can comment on or make changes to this bug.
Description
•