Flickering while selecting text in columns

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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: 67752
You're right, that's a pretty bad bug in the selection-underline code.
Yeah, I was testing on mac too.  I wonder what the difference is...

In any case, patch coming up tomorrow for testing.
Ah,

> 5024     if (didHaveSelectionUnderline != aSelected ||
> 5025         (aSelected && CombineSelectionUnderlineRect(PresContext(), r))) {

should be:

> 5024     if ((!aSelected && didHaveSelectionUnderline) ||
> 5025         (aSelected && CombineSelectionUnderlineRect(PresContext(), r))) {

?
Assignee: nobody → roc
Created attachment 377313 [details] [diff] [review]
fix

This is fairly simple
Attachment #377313 - Flags: review?(dbaron)
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+
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.
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.
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.
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/282cbda01920
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
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.
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
Attachment #377313 - Flags: approval1.9.1? → approval1.9.1+
The branch code is actually different and doesn't need this fix.
Flags: blocking1.9.1+
Whiteboard: [needs 191 landing]
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.