Closed
Bug 491960
Opened 15 years ago
Closed 15 years ago
Flickering while selecting text in columns
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(1 file)
1.42 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
Visit http://weblogs.mozillazine.org/roc/ and select text across columns. It flickers and moves around. I'm pretty sure this is due to reflow being interrupted. 1) Selection shouldn't be causing reflows, we should track that down. 2) We probably shouldn't interrupt inside columns, since the intermediate states during balancing can be pretty far from the initial and final layout.
Comment 1•15 years ago
|
||
roc, is this on Mac? I can't seem to reproduce (and yes, I think the build I'm using here has the ireflow patch). > 1) Selection shouldn't be causing reflows, we should track that down. #0 PresShell::FrameNeedsReflow (this=0x126a400, aFrame=0x1600520, aIntrinsicDirty=eStyleChange, aBitToAdd=1024) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:3307 #1 0x11d137d0 in nsTextFrame::SetSelected (this=0x1600520, aPresContext=0x1481600, aRange=0x1e3ce7a0, aSelected=1, aSpread=eSpreadNone, aType=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsTextFrameThebes.cpp:5028 #2 0x11d138b1 in nsTextFrame::SetSelected (this=0x1616534, aPresContext=0x1481600, aRange=0x1e3ce7a0, aSelected=1, aSpread=eSpreadDown, aType=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsTextFrameThebes.cpp:5042 #3 0x11cfda04 in nsTypedSelection::selectFrames (this=0x1d1be690, aPresContext=0x1481600, aRange=0x1e3ce7a0, aFlags=1) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsSelection.cpp:4418 The relevant code: 5018 // If the selection state is changed in this content, we need to reflow 5019 // to recompute the overflow area for underline of spellchecking or IME if 5020 // their underline is thicker than normal decoration line. 5021 PRBool didHaveSelectionUnderline = 5022 !!(mState & TEXT_SELECTION_UNDERLINE_OVERFLOWED); 5023 nsRect r(nsPoint(0, 0), GetSize()); 5024 if (didHaveSelectionUnderline != aSelected || 5025 (aSelected && CombineSelectionUnderlineRect(PresContext(), r))) { 5026 PresContext()->PresShell()->FrameNeedsReflow(this, 5027 nsIPresShell::eStyleChange, 5028 NS_FRAME_IS_DIRTY); 5029 } didHaveSelectionUnderline == 0, of course. aSelected == 1, since we're getting selected. This was introduced in bug 392785. I think the test is bogus, and we should be checking whether the new selection is one that will draw underlines or something, but I'm not that familiar with this code. I'm fine with disabling interrupts inside columns, though. It's a pretty simple fix.
Blocks: ireflow
Assignee | ||
Comment 2•15 years ago
|
||
You're right, that's a pretty bad bug in the selection-underline code.
Assignee | ||
Comment 3•15 years ago
|
||
My test is on Mac.
Comment 4•15 years ago
|
||
Yeah, I was testing on mac too. I wonder what the difference is... In any case, patch coming up tomorrow for testing.
Comment 5•15 years ago
|
||
Ah, > 5024 if (didHaveSelectionUnderline != aSelected || > 5025 (aSelected && CombineSelectionUnderlineRect(PresContext(), r))) { should be: > 5024 if ((!aSelected && didHaveSelectionUnderline) || > 5025 (aSelected && CombineSelectionUnderlineRect(PresContext(), r))) { ?
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 7•15 years ago
|
||
This particular bug only manifests with interruptible reflow, but the general issue of causing reflows through normal selection is really bad, and we should fix it on branch.
Flags: blocking1.9.1?
Comment on attachment 377313 [details] [diff] [review] fix r+sr=dbaron. I'm assuming the idea here is that you're now only going to cause reflows if the selection underline can actually cause overflow.
Attachment #377313 -
Flags: superreview+
Attachment #377313 -
Flags: review?(dbaron)
Attachment #377313 -
Flags: review+
Assignee | ||
Comment 9•15 years ago
|
||
Yes.
Comment 10•15 years ago
|
||
Is the patch correct?? If the decoration rect doesn't need to grow up but there is selection overflow rect already, the patch creates unneeded reflow.
Assignee | ||
Comment 11•15 years ago
|
||
How can we be sure that the new selection overflow rect is the same as the old one in that case? If we can't be sure, we need to reflow.
Comment 12•15 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#4931 nsTextFrame::CombineSelectionUnderlineRect checks the current overflow rect has enough space or not. Oh, but the r param at this time is same as mRect of the frame, so, not overflow rect. It seems that there is no problem, sorry.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs landing]
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/282cbda01920
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee | ||
Updated•15 years ago
|
Attachment #377313 -
Flags: approval1.9.1?
Assignee | ||
Comment 14•15 years ago
|
||
Comment on attachment 377313 [details] [diff] [review] fix A pretty serious performance regression for selection which we should fix on 1.9.1 --- the fix is very simple.
Assignee | ||
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
Updated•15 years ago
|
Attachment #377313 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 15•15 years ago
|
||
The branch code is actually different and doesn't need this fix.
Flags: blocking1.9.1+
Whiteboard: [needs 191 landing]
Comment 16•15 years ago
|
||
Comment on attachment 377313 [details] [diff] [review] fix As per Roc's comment
Attachment #377313 -
Flags: approval1.9.1+
You need to log in
before you can comment on or make changes to this bug.
Description
•