Closed Bug 519361 Opened 13 years ago Closed 13 years ago

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


(Core :: General, defect)

Not set





(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)




(3 files, 1 obsolete file)

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.
Could we use ordinary reftests if had a pref that makes the caret solid
(not blink)?
There is a setCaretReadOnly on selectioncontroller that makes the caret solid, I used that in bug 475425.
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?
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.
We'll also need to modify reftest.js to enable drawing of the caret via flag to drawWindow.
Oops, in comments #3 and #4 I'm just saying what Mats said in comment #1. sorry.
Attached patch reftest.diffSplinter Review
Here's a work-in-progress reftest for bug 503531 for testing...
Attached patch wip1 (obsolete) — Splinter Review
Add a pref which forces the caret to not blink and set it in
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?
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.
Attached patch reftest2.diffSplinter Review
This reftest seems reliable (on Linux) without using setTimeout.
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...
That looks right. I'm not sure why the caret isn't drawing, but it shouldn't be difficult to figure out.
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):
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

(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.
Depends on: 520720
Attachment #404135 - Flags: review?(roc)
Attachment #403689 - Attachment is obsolete: true
Just as a note, I know there are still people (such as bz, and possibly dbaron), who run reftest without using (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.
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?
(In reply to comment #14)
Ok, I posted a note to about it.
Assignee: nobody → matspal
Closed: 13 years ago
Resolution: --- → FIXED
(In reply to comment #15)
> hmm, I do that too.

FWIW, "make reftest" in your objdir will invoke the same way the tinderboxes do.
I have something like this in my shell environment:
reftest ()
    python $OBJDIR/_tests/reftest/ $*

then use "reftest layout/reftests/bugs/my.list" for example.
I attached a patch on bug 522703 to make it so you can do "make reftest TEST_PATH=layout/reftests/bugs/my.list".
We could set the pref in reftest.js; we do that already for a color-management pref:

(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).)
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...
(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.
You need to log in before you can comment on or make changes to this bug.