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)

enhancement

Tracking

()

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.
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.
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?
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)
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.
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.
(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.
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)
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!
Product: Core → Core Graveyard
Product: Core Graveyard → Core
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.