Open
Bug 1366642
Opened 8 years ago
Updated 2 years ago
nsImageMap can miss focus/blur events between load event and first paint
Categories
(Core :: Layout: Images, Video, and HTML Frames, enhancement)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
NEW
People
(Reporter: bkelly, Unassigned)
References
Details
In bug 1363829 I've been struggling with persistent reftest failures in:
https://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/703186-1.html
After much debugging on the try search I have discovered the underlying issue. The test is roughly:
1. Wait for load event
2. setTimeout(f, 0)
3. Call Focus() on a <map><area></map> element
4. setTimeout(f, 0)
5. Verify the area is rendered as focused
My patches in bug 1363829 make setTimeout(f, 0) faster, so the timing changes here a bit.
In particular, the setTimeout(f, 0) in step (2) above can now happen before the first paint sometimes. It turns out that nsImageMap doesn't set its event listeners for focus/blur until the first paint:
https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsImageMap.cpp#856
This means the Focus() call in step (3) does not actually trigger invalidation or repaint.
I'm planning to work around the problem by making step (2) a requestAnimationFrame() instead.
Does anyone have ideas on a better way to handle this long term? It seems that this should work after the load event to me.
Reporter | ||
Comment 1•8 years ago
|
||
I could also do other lame workaround like making the test twiddle the "shape" or "coord" attributes. Those also apparently will trigger the initialization here.
Comment 2•8 years ago
|
||
Imagemaps aren't great in this respect, but we use the fact that a setTimeout from (or after) the load event should fire after the page has painted in many places actually. Are you changing that invariant?
Reporter | ||
Comment 3•8 years ago
|
||
This is the first I've heard of this invariant. I don't see anything in the code that previously guaranteed it. Can you point me to more examples?
Flags: needinfo?(tnikkel)
Reporter | ||
Comment 4•8 years ago
|
||
Some initial instrumentation does show that the setTimeout(f, 0) is firing before first paint when it fails. I'm adding some more to try to figure out what conditions are causing this.
Its also unclear to me what mechanism tries to guarantee first paint happens first. So far I don't see anything existing in the code to enforce this.
Reporter | ||
Comment 5•8 years ago
|
||
There is no timeout coallescing going on. So setTimeout(f, 0) is basically doing a NS_DispatchToMainThread(timerRunnable) in the load event. In the past it was doing nsITimer::Init(timerRunnable, zeroDelay). So the only difference is the overhead we used to require to round-trip through the TimerThread.
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> Imagemaps aren't great in this respect, but we use the fact that a
> setTimeout from (or after) the load event should fire after the page has
> painted in many places actually. Are you changing that invariant?
I asked Boris about the "setTimeout(0) in load event must fire after first paint" and he wasn't aware of it either:
http://logs.glob.uno/?c=mozilla%23content&s=23+May+2017&e=23+May+2017&h=reftest#c442067
I don't think this is actually a system invariant. Or if it was at one time, it hasn't been for a while.
Comment 7•8 years ago
|
||
Sorry, I was remembering things. What I was actually thinking of was that a setTimeout fired from the load event will run after paint suppression ends. Which is important for tests that say want to fire a click event, if they do it before paint suppression ends the click will hit nothing. This is because nsDocumentViewer::LoadComplete unsuppresses painting after firing the load event.
For imagemap tests in the past we've used "lame hacks" like you suggested.
Flags: needinfo?(tnikkel)
Reporter | ||
Comment 8•8 years ago
|
||
Ah, ok. I did put instrumentation in for paint suppression and verified it was not suppressed in the timeout. So that was not the problem here.
Thanks!
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•