519361 - Need a way to write regression tests of where the caret is painted

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mats Palmgren (inactive)]

Spawned of from bug 503531 comment 6 -
From Robert O'Callahan (:roc) (Mozilla Corporation):

One option for testing that would be perhaps generally useful would be to add
an API window.mozGetCaretBoundingClientRect() which returns the
viewport-relative bounding box of the caret.

An alternative which would work but is less convenient would be to add "caret
blink" as an invalidation reason in nsPaintRequest. Then if the caret is
visible you can wait for it to blink and get the invalidation rectangle which
tells you where the caret is.

Comment 1

[Mats Palmgren (inactive)]

Could we use ordinary reftests if had a pref that makes the caret solid
(not blink)?

Comment 2

[Martijn Wargers (dead)]

There is a setCaretReadOnly on selectioncontroller that makes the caret solid, I used that in bug 475425.
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionController.idl#134

Comment 3

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

reftests can't use enable-privilege I believe.

Martijn, I'm not sure if the test in bug 475425 would be reliable.

Maybe the easiest thing to do is to add a pref that disables caret blinking altogether and set that pref in the test profile?

Comment 4

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

We really really need this IMHO, right now we have basically no test coverage in a fragile and poorly understood area of our code.

At the moment I think reftests plus a pref to disable blinking seems the best way to go.

Comment 5

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

We'll also need to modify reftest.js to enable drawing of the caret via flag to drawWindow.

Comment 6

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

Oops, in comments #3 and #4 I'm just saying what Mats said in comment #1. sorry.

Comment 7

[Mats Palmgren (inactive)]

Attachment: reftest.diff

Here's a work-in-progress reftest for bug 503531 for testing...

Comment 8

[Mats Palmgren (inactive)]

Attachment: wip1

Add a pref which forces the caret to not blink and set it in runreftest.py.
Use the DRAWWINDOW_DRAW_CARET flag when calling drawWindow.

It works if I use reftest-wait and delay the className = '' with 50msec
(see previous attachment).  I don't think that should be needed.
Maybe it has something to do with the async nature of caret drawing?
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCaret.cpp#372

Comment 9

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

Seems to me if I set "ui.caretBlinkTime" to -1 in a trunk build, the caret stops blinking, so maybe we don't need a new pref here?

Not sure about why the delay is needed, that seems bad. It shouldn't be too hard to figure out why the drawWindow isn't painting the caret.

Comment 10

[Mats Palmgren (inactive)]

Attachment: reftest2.diff

This reftest seems reliable (on Linux) without using setTimeout.

Comment 11

[Mats Palmgren (inactive)]

Attachment: wip2, using ui.caretBlinkTime = -1

roc, this is what you suggested, right?
I tried hard to make a reftest that work with this patch, but failed.
Setting the pref manually and loading the individual reftest files works
correctly (caret is visible and doesn't blink), but when running it as
a reftest the reference snapshot always lacks a caret. I even tried with
a 5 second delay and I can clearly see the caret isn't there...

Comment 12

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

That looks right. I'm not sure why the caret isn't drawing, but it shouldn't be difficult to figure out.

Comment 13

[Mats Palmgren (inactive)]

I'm a bit tired now, but I want to write this down before I forget...

First bug, 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.

Did a quick fix for that by forcing a blink rate of 20ms until we
find painting is not suppressed anymore, then use specified blink
rate after that.  Still doesn't work though...

Second bug, the invalidation of the caret rect is ignored in the
widget layer: nsWindow::Invalidate calls CanBeSeen() which returns
false.  The widget in question is newly created and still has
mIsVisible=false because it has not recieved an OnVisibilityNotifyEvent
yet. To my surprise I see that we do get an expose event *before* the
visibility event.  This URL says that shouldn't happen (at least
not for an X window):
http://tronche.com/gui/x/xlib/events/window-state-change/visibility.html
I couldn't find any documentation regarding the same for GdkWindow.

I fixed that by setting mIsVisible=true in OnExposeEvent -- and that
fixed the problem with the missing caret.

I also believe the above is why there is often a delay before the caret
is displayed.  nsCaret is supposed to start with an on-cycle but with
the above bugs there can often be many cycles before it is displayed.
With the above fixes, the caret is displayed instantly on reload as
intended.

(Also, why do we unsuppress painting even though the root widget isn't
visible... it means all those paints will be wasted?)

I'll file a separate bug on the nsCaret bug above.  The second bug
is possibly bug 519524 so I'll post a separate patch for that there.
With those fixed, the "wip2" patch in this bug should be complete
I think.  I have only tested on Linux so far...

Also, I'm pretty sure there are reftests that will break when
we start painting the caret, we need to fix those first.

Comment 14

[(not currently active) Ted Mielczarek]

Just as a note, I know there are still people (such as bz, and possibly dbaron), who run reftest without using runreftest.py (just using firefox -reftest). If we're going to start adding default prefs to it  that tests will rely on, we might need to inform people that "firefox -reftest" is no longer a reliable or supported option.

Comment 15

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

hmm, I do that too.

maybe we should add a new option to drawWindow that forces the caret to render even if it's supposed to be blinked off?

Comment 16

[Mats Palmgren (inactive)]

(In reply to comment #14)
Ok, I posted a note to mozilla.dev.tech.layout about it.

Comment 17

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/9603902c1e8f

Comment 18

[(not currently active) Ted Mielczarek]

(In reply to comment #15)
> hmm, I do that too.

FWIW, "make reftest" in your objdir will invoke runreftest.py the same way the tinderboxes do.

Comment 19

[Mats Palmgren (inactive)]

I have something like this in my shell environment:
reftest ()
{
    python $OBJDIR/_tests/reftest/runreftest.py $*
}

then use "reftest layout/reftests/bugs/my.list" for example.

Comment 20

[(not currently active) Ted Mielczarek]

I attached a patch on bug 522703 to make it so you can do "make reftest TEST_PATH=layout/reftests/bugs/my.list".

Comment 21

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

We could set the pref in reftest.js; we do that already for a color-management pref:  http://hg.mozilla.org/mozilla-central/rev/4b2977a03aba

(I've sometimes wondered if that code could be improved by setting the pref on the default branch and clearing it as a user pref, such that it wouldn't ever write the pref to the prefs file (although it would clear it if it was already there, but that's probably ok for both of these).)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Was there a reason to set caretBlinktime to -1 (which means "blink pretty rarely") instead of 0?  As far as I can tell, 0 is the "never blink" value...

Comment 23

[Martijn Wargers (dead)]

(In reply to Boris Zbarsky [:bz] from comment #22)
> Was there a reason to set caretBlinktime to -1 (which means "blink pretty
> rarely") instead of 0?  As far as I can tell, 0 is the "never blink" value...

You're right, I used the ui.caretBlinkTime = 0 pref and then the caret stops blinking. I filed bug 1203082 to use 0 for this.