Closed Bug 549799 Opened 10 years ago Closed 9 years ago

test_hover fails with integrated view manager hierarchies


(Core :: Layout, defect)

Not set





(Reporter: roc, Assigned: tnikkel)




(1 file, 2 obsolete files)

I think I've figured out what's going on.

First, it seems to me that the tests like
  is(step8called, false, "step8 called only once (more than two cycles of oscillation)");
can never fail anymore because the onresize event handler always clears itself:
    "document.body.setAttribute('onresize', 'void(0)');" + 
    "setTimeout('" + str + "', 100)");
That seems like a problem in the test, maybe a regression from bug 457862.

Now for the actual test failures with the bug 130078 patches. nsViewManager::ProcessSynthMouseMoveEvent now runs on the view manager for the toplevel chrome window. It calls PresShell::DispatchSynthMouseMove on the presshell for that chrome window. DispatchSynthMouseMove watches for changes to mFrameConstructor->GetHoverGeneration(). However, there aren't any, because the hovered content in test_hover.html is in a subframe with a different presshell. So layout doesn't happen in DispatchSynthMouseMove, it happens later when further synthetic mouse moves are not suppressed.
I think the simple solution here is to make the nsCSSFrameConstructor mHoverGeneration a global variable. Does that sound OK?
Hmm, so that actually isn't enough. The problem is that PresShell::DispatchSynthMouseMove needs to flush reflows on all shells that contain hovered content. Just doing a Flush_Layout on the root presshell isn't enough to do that --- often the root presshell won't have anything that needs to be reflowed at all.
OK, I can fix that by tracking which presshells actually have hovered content in them, and flushing all of them.

But the test is still failing, and I think it's because with integrated view managers, any reflow in the chrome will trigger a synthetic mouse event for the content.
Attached patch does this help? (obsolete) — Splinter Review
FindViewContaining seems broken to me, does this diff help fix the problem here?
Comment on attachment 446136 [details] [diff] [review]
does this help?

No it doesn't help.
Attachment #446136 - Attachment is obsolete: true
The problem here is that the combined stop/reload button switches from a disable stop button to an enabled reload button using a 650 ms setTimeout here . This happens in the middle of the test. Changing the image causes us to eventually get to nsImageBoxFrame::AttributeChanged . It asks for a reflow because it is the src that has changed. This reflows the root frame of the root chrome document. After that reflow is done it asks the view manager for a synth mouse move. Without 130078 this synth mouse move does not affect the test because the view manager for the test is not linked to the root chrome documents view manager. But they are linked so this synth mouse move makes hover stop applying during the 500 ms wait between step4 and step5. Step5 expects hover to be applying when it starts and moves the mouse so that hover does not apply. It times out because it never gets the notification it is waiting for from hover going from applying to not applying.

I don't think the test can do anything to prevent reflows from happening the in parent document and triggering synth mouse moves. I think we should just start the test off a 2 second timeout so that we avoid this stop/reload button issue.
I confirmed that making the combined stop/reload a 0 ms timeout, or a 9 second timeout fixes the test.
Blocks: 343396
Attached patch make the test wait to start (obsolete) — Splinter Review
Assignee: roc → tnikkel
Attachment #469769 - Flags: review?(roc)
Attachment #469769 - Flags: review?(dbaron)
Actually I think a better approach, instead of adding a delay (which is a) slow and b) possibly unreliable) would be to run the content of the tests in its own toplevel window.
Attachment #469769 - Attachment is obsolete: true
Attachment #469785 - Flags: review?(roc)
Attachment #469785 - Flags: feedback?(dbaron)
Attachment #469769 - Flags: review?(roc)
Attachment #469769 - Flags: review?(dbaron)
Your patch does not contain hover_helper
I did something tricky: I hg mv'd test_hover.html to hover_helper.html first, then changed hover_helper.html, then added test_hover.html again. hg understood the adding of test_hover.html as just changing the old test_hover.html because I did it all in one queue.

The magic happens here:
diff --git a/layout/style/test/test_hover.html b/layout/style/test/hover_helper.html
copy from layout/style/test/test_hover.html
copy to layout/style/test/hover_helper.html
--- a/layout/style/test/test_hover.html
+++ b/layout/style/test/hover_helper.html
Comment on attachment 469785 [details] [diff] [review]
open test in new window

You are too clever for me
Attachment #469785 - Flags: review?(roc) → review+
Did you test that the test fails if you comment out the synth mouse move event code in the view manager?
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 469785 [details] [diff] [review]
open test in new window

sounds fine given comment 15; should have marked as such earlier
Attachment #469785 - Flags: feedback?(dbaron) → feedback+
I haven't addressed comment 15 yet. It came after I landed (it took me a while to paste the changeset link and resolve as fixed).
As expected the test fails if you comment out synth mouse move event code in the view manager.
You need to log in before you can comment on or make changes to this bug.