Closed Bug 389462 Opened 15 years ago Closed 13 years ago

Columns in percent-width div don't adjust correctly when window is resized.

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: fantasai.bugs)

References

Details

(Keywords: testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
Found this bug while coming up with testcases for bug 387876.

Steps to reproduce:
-------------------
 - Load testcase
 - Resize window horizontally, putting lots of space between columns.
 - Refresh the page

Expected behavior:
------------------
 Layout after window resize should match layout after refresh. (It does in FF 2.0)

Observed behavior:
------------------
 Layout after window resize is *different* from layout after refresh.

At first glance, it seems that on page-load, we try to preserve the "-moz-column-gap: 10px;" rule, while letting the columns be bigger or smaller than their prescribed 300px.  However, on a page resize, it looks like we do the opposite -- we keep the column width fixed, while assigning any extra space to the gap.

Occasionally, we seem to assign "negative space" to the gap, making the columns overlap, which is definitely bad.
Keywords: testcase
Attached file height reflow testcase
Attached patch patch (obsolete) — Splinter Review
The height reflow testcase is actually a distinct, but related problem. In both cases, we don't tell the columns to reflow when there's a change in the column size constraints. Here's a patch that fixes both! I believe the code is correct. I'm not sure if the AddStateBits call belongs here or somewhere in nsHTMLReflowState.
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
Attachment #385536 - Flags: superreview?(roc)
Attachment #385536 - Flags: review?(roc)
+  // Reflow our last line if our availableHeight has increased
+  if (aState.mReflowState.availableHeight != NS_UNCONSTRAINEDSIZE
+      && GetNextInFlow() && aState.mReflowState.availableHeight > mRect.height) {

Why do we need to do this?

+    PRBool skipIncremental = !(aReflowState.ShouldReflowAllKids())

Outer parens unnecessary

The rest looks good, thanks, but we need some tests.
Because blocks otherwise don't pull up content from their next-in-flow if the available height has increased.
OK. I think a better fix would be to modify the skipPull logic further down in ReflowDirtyLines.
That doesn't work when you have nested elements.
Attached patch updated patchSplinter Review
Removes extra parentheses.
Attachment #385536 - Attachment is obsolete: true
Attachment #385683 - Flags: superreview?(roc)
Attachment #385683 - Flags: review?(roc)
Attachment #385536 - Flags: superreview?(roc)
Attachment #385536 - Flags: review?(roc)
Blocks: 372772
Comment on attachment 385683 [details] [diff] [review]
updated patch

OK I'm convinced, but can you add a comment explaining that we need to reflow the last line in case it's a block that can pull up more lines?
Attachment #385683 - Flags: superreview?(roc)
Attachment #385683 - Flags: superreview+
Attachment #385683 - Flags: review?(roc)
Attachment #385683 - Flags: review+
roc, does this reftest look ok? It's got a timeout because I couldn't figure out how to get the misrendering to trigger any other way...
Attachment #385897 - Flags: review?(roc)
dbaron helped me with the reftests (Thanks, dbaron!) Fix checked in
  http://hg.mozilla.org/mozilla-central/rev/a13eed3a20e7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Duplicate of this bug: 417487
You need to log in before you can comment on or make changes to this bug.