Closed
Bug 549799
Opened 15 years ago
Closed 14 years ago
test_hover fails with integrated view manager hierarchies
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: tnikkel)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
roc
:
review+
dbaron
:
feedback+
|
Details | Diff | Splinter Review |
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:
iframe.contentDocument.body.setAttribute("onresize",
"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.
Reporter | ||
Comment 1•15 years ago
|
||
I think the simple solution here is to make the nsCSSFrameConstructor mHoverGeneration a global variable. Does that sound OK?
Comment 2•15 years ago
|
||
Yes, sounds fine.
Reporter | ||
Comment 3•15 years ago
|
||
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.
Reporter | ||
Comment 4•15 years ago
|
||
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.
Assignee | ||
Comment 5•15 years ago
|
||
FindViewContaining seems broken to me, does this diff help fix the problem here?
Assignee | ||
Comment 6•14 years ago
|
||
Comment on attachment 446136 [details] [diff] [review]
does this help?
No it doesn't help.
Attachment #446136 -
Attachment is obsolete: true
Assignee | ||
Comment 7•14 years ago
|
||
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 http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4498 . This happens in the middle of the test. Changing the image causes us to eventually get to nsImageBoxFrame::AttributeChanged http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsImageBoxFrame.cpp#165 . 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.
Assignee | ||
Comment 8•14 years ago
|
||
I confirmed that making the combined stop/reload a 0 ms timeout, or a 9 second timeout fixes the test.
Assignee | ||
Comment 9•14 years ago
|
||
Assignee: roc → tnikkel
Attachment #469769 -
Flags: review?(roc)
Assignee | ||
Updated•14 years ago
|
Attachment #469769 -
Flags: review?(dbaron)
Reporter | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
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)
Reporter | ||
Comment 12•14 years ago
|
||
Your patch does not contain hover_helper
Assignee | ||
Comment 13•14 years ago
|
||
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
Reporter | ||
Comment 14•14 years ago
|
||
Comment on attachment 469785 [details] [diff] [review]
open test in new window
You are too clever for me
Attachment #469785 -
Flags: review?(roc) → review+
Comment 15•14 years ago
|
||
Did you test that the test fails if you comment out the synth mouse move event code in the view manager?
Assignee | ||
Comment 16•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 17•14 years ago
|
||
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+
Assignee | ||
Comment 18•14 years ago
|
||
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).
Assignee | ||
Comment 19•14 years ago
|
||
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.
Description
•