Closed Bug 387876 Opened 13 years ago Closed 13 years ago

Columns in absolutely positioned div break when changed

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: dholbert)

References

Details

(Keywords: regression, testcase)

Attachments

(7 files, 13 obsolete files)

693 bytes, text/html
Details
355 bytes, text/html
Details
327 bytes, text/html
Details
438 bytes, text/html
Details
500 bytes, text/html
Details
3.97 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase
When the content of columns in an absolutely positioned div is changed the new content is laid out all wrong.
Keywords: regression
This works in Firefox 2.0 BTW.
For me, the paragraph appears in the third column in FF2.0.0.4 and in the second column in a recent build. I'm not sure I'd call that a regression.
Flags: blocking1.9? → blocking1.9+
fantasai: it works fine for me on winxp.

dbaron: doesn't seem to, no.
This shows a reduced version of the "disappearing title" issue in the original testcase.
Attached patch reftest (obsolete) — Splinter Review
Here's a reftest version of reduced testcase.
Here's a reference page for reduced test case.  Just has "height" specification removed, which doesn't change page initial appearance, but does prevent bug from occurring.
Assignee: nobody → dholbert
Attached patch patch 1 (fix disappearing text) (obsolete) — Splinter Review
This patch fixes the disappearing text issue.

Basically, in nsColumnSetFrame::ReflowChildren, we were never checking the NS_FRAME_HAS_DIRTY_CHILDREN bit when considering whether to reflow a child.  Now we do.
Nit: you should use NS_SUBTREE_DIRTY().
Comment on attachment 273487 [details] [diff] [review]
patch 1 (fix disappearing text)

You should probably use the NS_SUBTREE_DIRTY macro.  (It calls GetStateBits once, and tests against the | of the two bits.)

I think you'd want to use that for all four of these dirty-ness tests, actually.

I also don't understand the difference between skipIncremental and skipResizeHeightShrink.

roc should probably also review this.
Attachment #273487 - Flags: review?(dbaron) → review-
- Changed dirty checks to use NS_SUBTREE_DIRTY macro.
- Added the mySubtreeClean bool for minor optimization (similar to shrinkingHeightOnly bool), to prevent repeated / per-loop calls to NS_SUBTREE_DIRTY(this)
Attachment #273487 - Attachment is obsolete: true
(In reply to comment #10)
> I also don't understand the difference between skipIncremental and
> skipResizeHeightShrink.

From what I can tell, those variables were intended as follows:

 - skipIncremental says: 
    IF the columnSet, the current column, and the next column are all clean
    THEN I don't need to reflow the current column
    BECAUSE there's no way its contents / dimensions would change, and so it shouldn't need a reflow.

 - skipResizeHeightShrink says 
     IF the columnSet and the current column are clean
       AND if the reflow is just due to a height-shrink 
       AND the current child fits in the new height
     THEN we don't need to reflow the current column
     BECAUSE the column's contents/dimensions shouldn't change, and so it shouldn't need a reflow.
Comment on attachment 273513 [details] [diff] [review]
patch 2 (fix disappearing text, using NS_SUBTREE_DIRTY)

I really should review this...
Attachment #273513 - Flags: superreview+
Attachment #273513 - Flags: review?(dbaron)
Attachment #273513 - Flags: review+
Thanks roc -- was gonna mark you as 2nd reviewer after hearing back from dbaron on updated patch, but you got to it first. :)
Attached patch reftests (as patch) (obsolete) — Splinter Review
2 reftests, which test tweaking the first column and the last column.

My initial patch fixed the first test, but not the second.  
Final patch fixes both tests.
Attachment #273198 - Attachment is obsolete: true
Attachment #273654 - Flags: review?(roc)
Is this patch really right?  !(GetStateBits() & NS_FRAME_IS_DIRTY) and mySubtreeClean are not the same thing, and in this case, I think you're destroying the optimization.
(In reply to comment #17)

I think Eli's right.  Roc, what do you think?  Is there any reason we'd want/need to check NS_SUBTREE_DIRTY on the top-level ColSetFrame here, or should the NS_FRAME_IS_DIRTY bit be enough for that frame?

Posting a new version of the patch that preserves the original checks for NS_FRAME_IS_DIRTY on the top-level ColSetFrame, rather than checking mySubtreeClean (aka !NS_SUBTREE_DIRTY).

This new patch passes the reftests.
Attachment #273662 - Flags: review?(roc)
Comment on attachment 273662 [details] [diff] [review]
patch 3 (alternate version, preserving original check for NS_FRAME_IS_DIRTY on top-level frame)

yes sorry, eli is right.
Attachment #273662 - Flags: superreview+
Attachment #273662 - Flags: review?(roc)
Attachment #273662 - Flags: review+
For the reftests, could your tweak() function actually change the text? If we introduced an optimization for appending empty strings, these tests would stop working.

Also, could we have the reference pages not do dynamic changes? Generally I think it's a good idea to simplify the reference pages as much as possible.
Attached patch reftests (updated) (obsolete) — Splinter Review
Thanks for the comments, roc -- how do these look?
Attachment #273654 - Attachment is obsolete: true
Attachment #273694 - Flags: review?(roc)
Attachment #273654 - Flags: review?(roc)
Comment on attachment 273694 [details] [diff] [review]
reftests (updated)

They look good, assuming they pass :-)
Attachment #273694 - Flags: superreview+
Attachment #273694 - Flags: review?(roc)
Attachment #273694 - Flags: review+
Yup. :)  Double-checked to make sure they fail pre-patch and pass post-patch.
Marking checkin-needed, for these two attachments:
 - attachment 273662 [details] [diff] [review], "patch 3"
 - attachment 273694 [details] [diff] [review], "reftests (updated)"

Whoever checks in reftests, please also add these lines to  layout/reftests/bugs/reftest.list:
== 387876-1.html 387876-1-ref.html
== 387876-2.html 387876-2-ref.html

(I'd include this in patch, but the reftest.list file degrades quickly, so chances are good that patch wouldn't apply when it'd get checked in)
Keywords: checkin-needed
This bug's patch is actually mostly included in the patch for bug 379349 -- attachment 273898 [details] [diff] [review] -- which has landed.  The only part of this bug's patch that's *not* included in that other patch is this change:

     PRBool skipResizeHeightShrink = shrinkingHeightOnly
-      && !(child->GetStateBits() & NS_FRAME_IS_DIRTY)
+      && !NS_SUBTREE_DIRTY(child)
       && child->GetOverflowRect().YMost() <= aConfig.mColMaxHeight;

This was added for thoroughness (see comment #10), but I'm actually not sure that this change is needed.  It's definitely not necessary to fix this bug's testcases, at least, and I haven't come up with another testcase where this change would make a difference.

roc/dbaron, let me know if you still think we should make this change.

In any case, the disappearing title problem in this bug's original testcase is fixed after landing of patch for bug 379349.  

However, there's still the issue of the "Paragraph OK?" text appearing to the right of "Title OK", instead of below where I think it belongs. Looking into that now.
This patch fixes the text-in-wrong-column issue.
If shrinkingHeightOnly is true, then this's subtree must not be dirty, so child's subtree must also not be dirty. So I guess we can just remove that check.
D'oh! Very good point, roc -- we don't need that check at all.
Attachment #275800 - Flags: review?(roc)
Attachment #275800 - Attachment is patch: true
Attachment #275800 - Attachment mime type: text/x-patch → text/plain
Grouping my prev two patches together -- didn't really need to separate out that dirty check into its own patch.

This patch passes reftests.
Attachment #275723 - Attachment is obsolete: true
Attachment #275800 - Attachment is obsolete: true
Attachment #275809 - Flags: review?(roc)
Attachment #275800 - Flags: review?(roc)
Attached patch reftests (3 now) (obsolete) — Splinter Review
Adding one more reftest, for text-in-wrong-column issue, to previous reftest attachment.

"patch v2 for text in wrong column" (attachment 275809 [details] [diff] [review]) makes this pass.
Attachment #273694 - Attachment is obsolete: true
Attachment #275810 - Flags: review?(roc)
+  // -- the first line of the next-in-flow isn't dirty. (or there is
+  // no such line)

Ah, but what if there the next in flow has no lines, but *its* next in flow has a dirty one?

My patch in bug 390050 adds an nsBlockInFlowLineIterator which would be very useful here!
Posting testcase with more columns, for the situation roc described.
This patch addresses roc's last point about the next-in-flow's next-in-flow, using a nsBlockInFlowLineIterator as he suggests.  (Thanks for that roc -- it's quite handy. :)) 

This patch depends on roc's patch for bug 390050 (attachment 275997 [details]), for the implementation of nsBlockInFlowLineIterator.
Attachment #275809 - Attachment is obsolete: true
Attachment #275998 - Flags: review?(roc)
Attachment #275809 - Flags: review?(roc)
Attached patch reftests (3.5 now) (obsolete) — Splinter Review
Adding a second version of the third reftest, starting it with more columns.
Attachment #275810 - Attachment is obsolete: true
+    // Start lineIter at end_lines(), so that nsBlockInFlowLineIterator::Next()
+    // will take us to first line of my next-in-flow-chain

That's not right. nsBlockInFlowLineIterator should be initialized to a real line. I should add comments and assertions to it to make that clear. You should initialize it to point to the last line. If there are no normal lines in this block you can just bail out of this optimization.
Fixed the issue with end_lines() vs. last-line-of-block that roc pointed out in previous comment.
Attachment #275998 - Attachment is obsolete: true
Attachment #276142 - Flags: review?(roc)
Attachment #275998 - Flags: review?(roc)
(removing checkin-needed keyword, 'cause it doesn't apply right now)
Keywords: checkin-needed
+    line_iterator lineIter = this->end_lines();
+    if (lineIter == this->begin_lines()) {
+      lineIter--; // I have lines; step back from dummy iterator to last line.

Shouldn't this be != instead of ==?
> Shouldn't this be != instead of ==?

Right you are -- thanks for catching that.  Reposting patch with that fixed,
Attachment #276142 - Attachment is obsolete: true
Attachment #276657 - Flags: review?(roc)
Attachment #276142 - Flags: review?(roc)
roc: These are the same reftests you previously reviewed on this bug, just with 'reftest-wait' added on the HTML elements, and with the "3-ref", "3a", and "3b" files added.
Attachment #282154 - Flags: review?(roc)
"patch v5" checked in

Checking in nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.867; previous revision: 3.866
done
Checking in nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.43; previous revision: 3.42
done
(Leaving bug open until reftests reviewed/checked in)
Reftests checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.