Closed
Bug 284664
Opened 19 years ago
Closed 19 years ago
mouseout events lost moving mouse out of iframe
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: danm.moz, Assigned: roc)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(6 files, 1 obsolete file)
205 bytes,
text/html
|
Details | |
1.26 KB,
text/html
|
Details | |
466 bytes,
text/html
|
Details | |
882 bytes,
text/html
|
Details | |
924 bytes,
text/html
|
Details | |
19.73 KB,
patch
|
roc
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
mouseout event listeners on an image contained within an iframe shrink-wrapped around the image never fire. Seemingly minor, but it turns out the webpage structure necessary to see this bug is not uncommon. Testcase coming up. This regressed in the 20050224 build and is still regressed in the 20050303 build. It was caused by the "fix #2" patch in bug 125386. I'm assigning and cc:ing as suggested by that bug.
Keywords: regression
Updated•19 years ago
|
Flags: blocking1.8b2?
Assignee | ||
Comment 3•19 years ago
|
||
This testcase shows that in older builds, mouseover/mouseout didn't really work on IFRAMEs; moving into the framed document was counted as a mouseout of the container and sometimes wasn't noticed at all.
Assignee | ||
Comment 4•19 years ago
|
||
This patch comprehensively fixes the mouseover/mouseout problems around IFRAMEs. It fixes all the testcases in this bug. The basic idea is that when we do GenerateMouseEnterExit, we need to do two things: -- if the mouse is moving out of an element which has a subdocument attached, tell the subdocument that we're moving out of its moused-over content (and recurse if that subdocument's hovered content has a subdocument...) -- if our document is a subdocument, tell the parent document that we are now hovering over the element attached to the subdocument (and recurse if necessary). This may require the parent to move its hover away from some content, invoking the previous rule, so in general you can have quite a complex set of notifications firing here. But I've made the code as clear as I can. MaybeDispatchMouseEventToIframe was a hack that tried to do the DOM-event-firing part of the second rule, but didn't try to deal with hover or changing the state of the parent ESM, so it's completely subsumed by this fix.
Attachment #176642 -
Flags: superreview?(bzbarsky)
Attachment #176642 -
Flags: review?(mats.palmgren)
Comment 5•19 years ago
|
||
Comment on attachment 176642 [details] [diff] [review] fix >Index: content/events/src/nsEventStateManager.cpp >+nsEventStateManager::DispatchMouseEvent(nsGUIEvent* aEvent, PRUint32 >- // If frame refs were cleared, we need to re-resolve the frame >- if (mClearedFrameRefsDuringEvent) { I assume that keeping track of mClearedFrameRefsDuringEvent here just made the code more complex without really speeding it up? >+nsEventStateManager::NotifyMouseOut(nsGUIEvent* aEvent) >+ // Before firing mouseout, check for recursion >+ if (mLastMouseOverElement == mFirstMouseOutEventElement) I'm not sure I follow the recursion-protection setup... worth documenting, perhaps (where the members are declared)? >+ mLastMouseOverElement = nsnull; >+ >+ // Turn recursion protection back off >+ mFirstMouseOutEventElement = nsnull; So when this method returns, we are guaranteed that mLastMouseOverElement and mFirstMouseOutEventElement are both null, right? >+nsEventStateManager::NotifyMouseOver(nsGUIEvent* aEvent, nsIContent* aContent) >+ NotifyMouseOut(aEvent); >+ // Potential havoc again. Reverify and take care. >+ if (mLastMouseOverElement == aContent) Therefore this is basically a null-check on aContent... Is that really what we want? The meat of the patch (going up and down the tree) looks ok, but I'd really like to sort out the recursion protection thing before signing off on it...
Assignee | ||
Comment 6•19 years ago
|
||
> I assume that keeping track of mClearedFrameRefsDuringEvent here just made the > code more complex without really speeding it up? Yeah and it requires having a primaryframe to pass in in the first place. Basically I don't see any need for speed in event processing. The only thing I'm concerned about is adding every moused-over text element to the primary frame map and even then I don't think it's something we should worry about. > I'm not sure I follow the recursion-protection setup... worth documenting, > perhaps (where the members are declared)? It doesn't completely prevent recursion. For mFirstMouseOverEventElement, all we're doing is remembering the last element for which we fired a NS_MOUSE_ENTER_SYNTH which hasn't finished processing yet (this is per document). (Similar for mouseout.) If we get a request to synthesize a mouseover on that element again, then we just ignore the request. This does not prevent cycles of length > 1, but neither did the previous code. Perhaps we should have a hash table which remembers all the elements on which we're currently processing mouseover events and ignore requests for all of them. > Therefore this is basically a null-check on aContent... Is that really what we > want? No, I was just being paranoid. We should drop that check.
Comment 7•19 years ago
|
||
> No, I was just being paranoid. We should drop that check.
Or perhaps you meant to compare to mFirstMouseOverEventElement, not to
mLastMouseOverElement? I don't see this patch ever comparing anything to
mFirstMouseOverEventElement...
Assignee | ||
Comment 8•19 years ago
|
||
No, it does here: + if (mLastMouseOverElement == mFirstMouseOutEventElement && + mFirstMouseOutEventElement)
Comment 9•19 years ago
|
||
That's mFirstMouseOutEventElement, not mFirstMouseOverEventElement (and boy, do we need better member names here). The only places in the patch that reference mFirstMouseOverEventElement are code that's being removed, and the two places where we set it...
Assignee | ||
Comment 10•19 years ago
|
||
Oh yeah :-) + if (mLastMouseOverElement == mFirstMouseOutEventElement && + mFirstMouseOutEventElement) That should be mFirstMouseOverEventElement, not Out.
Comment 11•19 years ago
|
||
Want to post a patch with that fixed, and I'll try to trace the recursion logic again?
Comment 12•19 years ago
|
||
Comment 13•19 years ago
|
||
Comment 14•19 years ago
|
||
Comment on attachment 176642 [details] [diff] [review] fix FWIW, the code looks fine to me. I'll leave the rest of the review to Boris ;-)
Attachment #176642 -
Flags: review?(mats.palmgren) → review?(bzbarsky)
Assignee | ||
Comment 15•19 years ago
|
||
Updated patch, with Mats' r+ (assuming from comments) carried over. I've documented the mFirstMouse* members so it should be clear we do what we're supposed to do, although as I mentioned before it's certainly not a complete prevention of recursion.
Attachment #176642 -
Attachment is obsolete: true
Attachment #178853 -
Flags: superreview?(bzbarsky)
Attachment #178853 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #176642 -
Flags: superreview?(bzbarsky)
Attachment #176642 -
Flags: review?(bzbarsky)
Comment 16•19 years ago
|
||
Comment on attachment 178853 [details] [diff] [review] fix #2 >Index: content/events/src/nsEventStateManager.cpp >+nsEventStateManager::NotifyMouseOver(nsGUIEvent* aEvent, nsIContent* >+ NotifyMouseOut(aEvent); >+ // Potential havoc again. Reverify and take care. >+ if (mLastMouseOverElement == aContent) >+ return; This still seems to be just a null-check on aContent (which would be better done up-front in the function, if it's needed). Did you mean to compare it to mFirstMouseOverEventElement? That seems to be what the old code was doing, no? sr=bzbarsky with that resolved in a reasonable way...
Attachment #178853 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 17•19 years ago
|
||
Oh yeah. That was pointless, I was just being uselessly paranoid, so I dropped it. We could put an additional recursion protection check there, but that's really a separate issue. What the old code was doing corresponds to the check at the top of the function. Checked in, thanks!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 18•19 years ago
|
||
The fix for this bug seem to have created a new bug. to reproduce the bug open Print Preview and point the mouse over the iframe. when coming back to the page, the content of the iframe disappears. you can use testcase 2 or 3 for this.
Comment 19•19 years ago
|
||
Could you please file a separate bug on that issue (and cc me and Robert)?
Comment 20•19 years ago
|
||
(In reply to comment #19) > Could you please file a separate bug on that issue (and cc me and Robert)? done. see Bug 288873
This also caused regression bug 288775.
Depends on: 288775
Depends on: 288873
Updated•19 years ago
|
Flags: blocking1.8b2?
Comment 22•19 years ago
|
||
Could this have caused bug 297561?
Updated•19 years ago
|
Comment 23•19 years ago
|
||
Comment on attachment 178853 [details] [diff] [review] fix #2 >- parentShell->HandleDOMEventWithTarget(docContent, &event, &status); >- } >+ nsIPresShell *parentShell = parentDoc->GetShellAt(0); >+ if (parentShell) { >+ nsEventStateManager* parentESM = >+ NS_STATIC_CAST(nsEventStateManager*, parentShell->GetPresContext()->EventStateManager()); >+ parentESM->NotifyMouseOver(aEvent, docContent); > } > } > } >-} >+ // Firing the DOM event in the parent document could cause all kinds of havoc. Indeed. HandleDOMEventWithTarget temporarily sets the parentESM's current event target, but importantly clears it when done. DispatchMouseEvent (which you call from NotifyMouseOver) does not clear the event target. The child's call doesn't matter because it's called from HandleDOMEventWithTarget in the first place.
Comment 25•18 years ago
|
||
(In reply to comment #24) > This probably caused bug > Don't leave us guessing. ;-)
Comment 26•18 years ago
|
||
"View Bug Activity" shows that bug 307767 was added to the "depends on" list at the time of the comment, so that would be my guess ;-)
You need to log in
before you can comment on or make changes to this bug.
Description
•