520720 - The caret isn't displayed directly after a reload (and may not display at all)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

The caret isn't displayed directly after a reload (and may not display
at all if ui.caretBlinkTime = -1).  See testcase in bug 519361.

The problem is when we get the first nsCaret::DrawCaret call painting is
still suppressed, so nsCaret::MustDrawCaret returns false and we
don't even try to complete DrawCaret (leaving mDrawn false).
When building display lists we call GetCaretFrame() which returns
nsnull when !mDrawn, so the caret is not going to be painted until
we get a callback from the blink timer, which will take a while
when using ui.caretBlinkTime = -1.

The suggested fix is to use a bit to remember that the first draw
was suppressed and setup the timer with a short delay and retry.
Sort of polling when the shell is unsuppressed.

There's also bug 519524 (on Linux) which blocks this bug.

Then I discovered there's a race condition which only occurs when
running reftests -- in PresShell::RenderDocument() there's no guarantee
the caret has woken up and seen painting is unsuppressed and
entered its mDrawn state yet.

Comment 1

[Mats Palmgren (inactive)]

Attachment: wip

Add nsCaret::mPendingDraw bit, set it when painting is suppressed,
use a fixed short (100ms) time instead of mBlinkRate when it's set.

Make nsCaret::CheckCaretDrawingState() initiate a draw if mPendingDraw
is set, so we have a way to "flush" the timer from outside.

Make PresShell::RenderDocument() flush the caret when RENDER_CARET
is requested.

The patch is cooking on the TryServer, and I need to review it a bit
more myself.  In particular, the consumers of DrawCaret(aInvalidate)
that gives PR_FALSE as argument...

Comment 2

[Mats Palmgren (inactive)]

Attachment: patch rev. 2

I lowered the timer from 100ms to 50ms which for me gives a visibly more
immediate response.

I investigated the DrawCaret(PR_FALSE) call, there is only one from
nsCaret::UpdateCaretPosition() which is called from
PresShell::WillDoReflow()/DidDoReflow() for bug 334649.
I didn't see any calls to UpdateCaretPosition with painting suppressed
but even if that would occur I think the patch works as is.

Comment 3

[Mats Palmgren (inactive)]

I forgot to mention, the first patch did pass regression tests on TryServer
(to my surprise), together with patches for bug 519361 and bug 509278.
So I think we have what we need to start writing those caret regression tests!

Comment 4

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

Wouldn't it be simpler (and faster) to have nsPresShell::UnsuppressAndInvalidate call CheckCaretDrawingState instead of having the caret poll like this?

Comment 5

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

Then I suspect you wouldn't need the code you've added to PresShell::RenderDocument.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

That's much better indeed.

Comment 7

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/53f1a647625c