387876 - Columns in absolutely positioned div break when changed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: testcase

When the content of columns in an absolutely positioned div is changed the new content is laid out all wrong.

Comment 1

[Jonathan Watt [:jwatt]]

This works in Firefox 2.0 BTW.

Comment 2

[fantasai]

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.

Comment 3

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

Does bug 364680 help?

Comment 4

[Jonathan Watt [:jwatt]]

fantasai: it works fine for me on winxp.

dbaron: doesn't seem to, no.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase (disappearing text)

This shows a reduced version of the "disappearing title" issue in the original testcase.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: reftest

Here's a reftest version of reduced testcase.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase: reference

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.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch 1 (fix disappearing text)

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.

Comment 9

[Eli Friedman]

Nit: you should use NS_SUBTREE_DIRTY().

Comment 10

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

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.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch 2 (fix disappearing text, using NS_SUBTREE_DIRTY)

- 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)

Comment 12

[Daniel Holbert [:dholbert]]

(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 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's right.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 273513 [details] [diff] [review]
patch 2 (fix disappearing text, using NS_SUBTREE_DIRTY)

I really should review this...

Comment 15

[Daniel Holbert [:dholbert]]

Thanks roc -- was gonna mark you as 2nd reviewer after hearing back from dbaron on updated patch, but you got to it first. :)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: reftests (as patch)

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.

Comment 17

[Eli Friedman]

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.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: patch 3 (alternate version, preserving original check for NS_FRAME_IS_DIRTY on top-level frame)

(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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: reftests (updated)

Thanks for the comments, roc -- how do these look?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 273694 [details] [diff] [review]
reftests (updated)

They look good, assuming they pass :-)

Comment 23

[Daniel Holbert [:dholbert]]

Yup. :)  Double-checked to make sure they fail pre-patch and pass post-patch.

Comment 24

[Daniel Holbert [:dholbert]]

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)

Comment 25

[Daniel Holbert [:dholbert]]

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.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase #2 (text in wrong column)

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: patch v1 for text in wrong column

This patch fixes the text-in-wrong-column issue.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: patch (removing unnecessary dirty check)

D'oh! Very good point, roc -- we don't need that check at all.

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: patch v2 for text in wrong column (includes removal of unnecessary dirty check)

Grouping my prev two patches together -- didn't really need to separate out that dirty check into its own patch.

This patch passes reftests.

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: reftests (3 now)

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.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // -- 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!

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase #3 (more columns)

Posting testcase with more columns, for the situation roc described.

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: patch v3 for column issue (using nsBlockInFlowLineIterator)

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.

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: reftests (3.5 now)

Adding a second version of the third reftest, starting it with more columns.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // 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.

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: patch v4 for column issue (using last line of block, instead of end_lines())

Fixed the issue with end_lines() vs. last-line-of-block that roc pointed out in previous comment.

Comment 38

[Daniel Holbert [:dholbert]]

(removing checkin-needed keyword, 'cause it doesn't apply right now)

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    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 ==?

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: patch v5 for column issue (flipped == to !=)

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

Right you are -- thanks for catching that.  Reposting patch with that fixed,

Comment 41

[Daniel Holbert [:dholbert]]

Attachment: reftests (added 'reftest-wait')

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.

Comment 42

[Daniel Holbert [:dholbert]]

"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

Comment 43

[Daniel Holbert [:dholbert]]

(Leaving bug open until reftests reviewed/checked in)

Comment 44

[Daniel Holbert [:dholbert]]

Reftests checked in.