Closed
Bug 387876
Opened 18 years ago
Closed 17 years ago
Columns in absolutely positioned div break when changed
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: jwatt, Assigned: dholbert)
References
Details
(Keywords: regression, testcase)
Attachments
(7 files, 13 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
When the content of columns in an absolutely positioned div is changed the new content is laid out all wrong.
Reporter | ||
Updated•18 years ago
|
Keywords: regression
Reporter | ||
Comment 1•18 years ago
|
||
This works in Firefox 2.0 BTW.
Updated•18 years ago
|
Blocks: reflow-refactor
Flags: blocking1.9?
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.
Updated•18 years ago
|
Flags: blocking1.9? → blocking1.9+
Reporter | ||
Comment 4•17 years ago
|
||
fantasai: it works fine for me on winxp.
dbaron: doesn't seem to, no.
Assignee | ||
Comment 5•17 years ago
|
||
This shows a reduced version of the "disappearing title" issue in the original testcase.
Assignee | ||
Comment 6•17 years ago
|
||
Here's a reftest version of reduced testcase.
Assignee | ||
Comment 7•17 years ago
|
||
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 | ||
Updated•17 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Attachment #273487 -
Flags: review?(dbaron)
Comment 9•17 years ago
|
||
Nit: you should use NS_SUBTREE_DIRTY().
Comment 10•17 years ago
|
||
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-
Assignee | ||
Comment 11•17 years ago
|
||
- 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
Assignee | ||
Comment 12•17 years ago
|
||
(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.
Assignee | ||
Updated•17 years ago
|
Attachment #273513 -
Flags: review?(dbaron)
That's right.
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+
Assignee | ||
Comment 15•17 years ago
|
||
Thanks roc -- was gonna mark you as 2nd reviewer after hearing back from dbaron on updated patch, but you got to it first. :)
Assignee | ||
Comment 16•17 years ago
|
||
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)
Comment 17•17 years ago
|
||
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.
Assignee | ||
Comment 18•17 years ago
|
||
(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+
Assignee | ||
Updated•17 years ago
|
Attachment #273513 -
Attachment is obsolete: true
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.
Assignee | ||
Comment 21•17 years ago
|
||
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+
Assignee | ||
Comment 23•17 years ago
|
||
Yup. :) Double-checked to make sure they fail pre-patch and pass post-patch.
Assignee | ||
Comment 24•17 years ago
|
||
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
Assignee | ||
Comment 25•17 years ago
|
||
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.
Assignee | ||
Comment 26•17 years ago
|
||
Assignee | ||
Comment 27•17 years ago
|
||
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.
Assignee | ||
Comment 29•17 years ago
|
||
D'oh! Very good point, roc -- we don't need that check at all.
Attachment #275800 -
Flags: review?(roc)
Assignee | ||
Updated•17 years ago
|
Attachment #275800 -
Attachment is patch: true
Attachment #275800 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Updated•17 years ago
|
Attachment #273662 -
Attachment is obsolete: true
Assignee | ||
Comment 30•17 years ago
|
||
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)
Assignee | ||
Comment 31•17 years ago
|
||
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!
Assignee | ||
Comment 33•17 years ago
|
||
Posting testcase with more columns, for the situation roc described.
Assignee | ||
Updated•17 years ago
|
Attachment #275810 -
Flags: review?(roc)
Assignee | ||
Comment 34•17 years ago
|
||
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)
Assignee | ||
Comment 35•17 years ago
|
||
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.
Assignee | ||
Comment 37•17 years ago
|
||
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)
Assignee | ||
Comment 38•17 years ago
|
||
(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 ==?
Assignee | ||
Comment 40•17 years ago
|
||
> 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)
Attachment #276657 -
Flags: superreview+
Attachment #276657 -
Flags: review?(roc)
Attachment #276657 -
Flags: review+
Assignee | ||
Comment 41•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #275999 -
Attachment is obsolete: true
Assignee | ||
Comment 42•17 years ago
|
||
"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
Assignee | ||
Comment 43•17 years ago
|
||
(Leaving bug open until reftests reviewed/checked in)
Attachment #282154 -
Flags: review?(roc) → review+
Assignee | ||
Comment 44•17 years ago
|
||
Reftests checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•