Closed Bug 284664 Opened 19 years ago Closed 19 years ago

mouseout events lost moving mouse out of iframe

Categories

(Core :: DOM: Events, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: danm.moz, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

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.
Attached file used by testcase
Attached file testcase
Testcase shows bug on builds beginning 24 Feb
Keywords: regression
Flags: blocking1.8b2?
Attached file testcase
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.
Attached patch fix (obsolete) — Splinter Review
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)
Blocks: 285185
Blocks: 271389
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...
> 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.
Blocks: 286968
> 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...
No, it does here:

+  if (mLastMouseOverElement == mFirstMouseOutEventElement &&
+      mFirstMouseOutEventElement)
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...
Oh yeah :-)

+  if (mLastMouseOverElement == mFirstMouseOutEventElement &&
+      mFirstMouseOutEventElement)

That should be mFirstMouseOverEventElement, not Out.
Want to post a patch with that fixed, and I'll try to trace the recursion logic
again?
Attached file Testcase #4
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)
Attached patch fix #2Splinter Review
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+
Attachment #176642 - Flags: superreview?(bzbarsky)
Attachment #176642 - Flags: review?(bzbarsky)
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+
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
Blocks: 274736
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.
Could you please file a separate bug on that issue (and cc me and Robert)?
(In reply to comment #19)
> Could you please file a separate bug on that issue (and cc me and Robert)?

done. see Bug 288873
Flags: blocking1.8b2?
Depends on: 291443
Could this have caused bug 297561?
Blocks: 298477
No longer blocks: 298477
Depends on: 298477
Depends on: 306149
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.
Depends on: 305706
This probably caused bug 
Depends on: 307767
Depends on: 300784
Depends on: 338718
(In reply to comment #24)
> This probably caused bug 
> 

Don't leave us guessing. ;-)
"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.