103055 - onmouseout event fired incorrectly, moving mouse over text counted as onmouseout

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P1)

Description

[Chris Bolt]

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4+)
Gecko/20011003

The attached file has four tables with onmouseover and onmouseout events.

Test #1:

- Move your cursor over the text, and leave it there. The onmouseout alert()
comes up. It shouldn't. It's like Mozilla counts moving the mouse over the text
in the cell as moving the mouse out of the cell.

Test #2:

- Move your cursor to just under the table, then slowly, pixel by pixel, move it
up until the background changes, then move it back down. The alert fires after
the cursor is moved out, which is good, but the background doesn't change back.

Test #3:

- Without an alert(), test #3 is fine and works the same as IE 5.

Test #4:

- Without an alert(), the onmouseover event doesn't even appear to be fired. The
background color isn't changed back in Mozilla, but is in IE 5.

Comment 1

[Chris Bolt]

Attachment: Test case

Comment 2

[Zach Lipton [:zach]]

I get the same thing with IE 5.1 on mac os x (without the background 
issue which is another bug), is this a moz bug?

Comment 3

[Christopher Hoess (gone)]

I have a nagging feeling that this is a dupe, but I could be wrong.  I
definitely think the background problem is new, however.

Comment 4

[Christian Reis]

Can confirm this on Linux 2001100308. The issue here is twofold though it seems
connected:

a) onmouseout() is being called when moving from table padding into the text
inside a TD. this is pretty wrong.
b) the colour isn't being changed on onmouseout()

there is of course the issue that the TD background and TABLE background event
handling are different (i.e. test 3 and test 4 behave differently). I think this
is a different issue, but since it seems reproducible here, let's have the
events people give their opinion.

Fabian, what's your opinion?

Comment 5

[Christian Reis]

Sorry for accepting that, jesus, a reload just left it pre-selected and I didn't
notice when posting comment. That's totally evil :(( Sorry.

Comment 6

[Fabian Guisset]

We have a new select guru, and it's not me ;-)
John is this fixed with that patch of yours? :-)

Comment 7

[Ed Sinek]

Attachment: HTML page showing errors when passing mouse over <a> text

When viewing the attachment in Mozilla, move the mouse over the 6 bars.  The
<a> text should underline whenever the <td> is 'moused over.'  When you run the
mouse over the <a> text, the onmouseout handler doesn't seem to always get run
because you'll see the onmouseover effect, but it does not revert to the
onmouseout effect when the mouse exits the area.

Comment 8

[Jesse McCarthy]

You're not planning on fixing this until 1.1?!?  What, are you f#%$#*$ crazy? 
By your own admission that wouldn't be for _years_.  Don't you think there are a
_few_ people using this event handler?  My god this browser is a fantastic peice
of garbage.  You might as well be honest and, if you plan to continue this
bumbling project, advertise Mozilla as a collection of bugs and set up an
immensely complicated tracking system to allow people to report the rare
occurrence when they might stumble upon a fragment of browser.

I too am experiencing this crippling defect in Netscape 6.1 / Win98.

If this is the best you can do, I'll be happy to see you get plowed under, and
believe me, it won't be long.

Comment 9

[Fabian Guisset]

Jesse, there are other ways to make us know that this bug is important to you
that to rant like you did. Give us some URL's of real sites that use this
(important sites helps) and an objective explanation of why this is so important
and this bug might be retargetted.
Oh and btw, patches welcome :-) 

Comment 10

[Jesse McCarthy]

"Real", important sites?  My site is most important to me (and it's real, I 
guess as opposed to an imagingary site, which might go nicely with this 
imaginary browser).

Spare me this "objective explanation" BS, I'm assuming you know perfectly well 
how commonly used and important this event handler is and how detrimental it is 
for it to be screwed up, and if you don't, then you really couldn't have 
anything meaningful to say about this.

Setting aside for a moment the absurdity of the idea that I, a _web_ developer -
- not a software developer, should essentially write the web browser before I 
can write a web page, how would I even know where to begin? there are over 
30,000 files here.

Comment 11

[Chris Bolt]

I *think* this bug affects the dynamic menus at the left of www.bell.ca.

Attaching a simplified testcase, note how moving the mouse from the right side
to the left then back to the right again mis-highlights things (the background
problem). The second example with alert()s shows how the events are fired.

Could this be related to bug 5693 except with scripting?

Comment 12

[Chris Bolt]

Attachment: bell.ca testcase

Comment 13

[Vladimir Ermakov]

*** Bug 134415 has been marked as a duplicate of this bug. ***

Comment 14

[Doron Rosenberg (IBM)]

I've run into this before, this can cause flickering in menus which has been
reported as evangelism issues into bugscape.

cc: netscape technology evangelism folks

Comment 15

[Doron Rosenberg (IBM)]

nominating for topembed

Comment 16

[Doron Rosenberg (IBM)]

Attachment: yet another testcase (really small!)

This testcase is a really simple one - hovering over the text causes an alert
it shouldn't be causing.

Comment 17

[Samuel Sieb]

An interesting effect with the bell.ca testcase is that when you move the mouse
from the text to the outer part, you get a <td>onmouseout, then a <td>onmouseover.

Comment 18

[/\/\arcio Galli]

Is this going to fix the cases with links? there are many implementation of
menus with text as links. We need to take care of both cases since bug 128097 is
wontfix.

Comment 19

[John Keiser (jkeiser)]

At least from the first testcase, it looks to me like it's the alert that's
affecting things.  #3 and #4 (without alerts) work correctly in 2002080811
(admittedly an old build but recent enough).

Comment 20

[James Gurney]

Attachment: testcase with style change

This is not just with alert(). Attached testcase makes a change to the style,
and this also occurs before the mouse leaves the td.

Comment 21

[James Gurney]

Attachment: Test case with style change

Whoops, testcase was broken. This should work. Tested here with 2002082608
incidentally.

Comment 22

[Andrew Schultz]

*** Bug 169461 has been marked as a duplicate of this bug. ***

Comment 23

[saari (gone)]

->bryner, high priority

Comment 24

[Cédric Marcone]

Attachment: explicit testcase

This bug begins to be really critical. Please help us brave moz developers

Comment 25

[John Keiser (jkeiser)]

I have a fix, reassigning.

Comment 26

[John Keiser (jkeiser)]

Attachment: Patch

OK, this patch requires a little explanation.  It:
(1) fixes this bug by checking for the first element parent of the targeted
content (and does all compares and such against that element parent rather than
the node).  Thus it does not ever fire mouseover / mouseout at anything but an
element.
(2) changes most (but not all yet) nsIContent*'s in nsEventStateManager into
nsCOMPtr's
(3) factors out mouse enter/exit event firing code into two methods,
FireMouseEvent() and MaybeFireAtIframe()
(4) sets :hover *before* onmouseover is fired.	This is done to make my life a
tiny bit easier, need to determine whether it matters or not.

It has been tested against this bug, and this bug, and this bug.  And I've been
surfing a while with it--XUL menus and friends work, so it seems like things
are OK, but I'll ask around for more :hover and mouseover/mouseout testcases.

Comment 27

[John Keiser (jkeiser)]

Attachment: Patch v1.1

This is the same as the last, except I made the new method names more verbose
and nsCOMPtr-ized the other nsIContent*'s and nsIDocument*'s as well.  Also
removed a completely unused nsIWidget*.  I have decided it's worth the risk to
have :hover set *before* onmouseout--since it's a non-cancellable event like
onchange, it probably ought to set state before it fires anyhow.  I'll watch
hover bugs after this lands.

Comment 28

[John Keiser (jkeiser)]

Comment on attachment 108696 [details] [diff] [review]
Patch v1.1

For your reviewing pleasure, I have uploaded the patch to Patch Viewer at
http://landfill.bugzilla.org/bz176656/attachment.cgi?id=109&action=prettyview .


Much of the patch is simply the move to nsCOMPtr and is reviewable by rote. 
Let me know if you want me to do that in a separate bug; I'd prefer to finish
it in this one if possible, but whatever gets it reviewed I'll do :)

Comment 29

[Brian Ryner (not reading)]

Comment on attachment 108696 [details] [diff] [review]
Patch v1.1

There's some risk here, but I think it's worth it for the cleanup, if jkeiser
is willing to watch for and fix regressions.

There are a couple of places where the indenting of closing braces seems to be
off. Fix those and r=bryner.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 108696 [details] [diff] [review]
Patch v1.1

- In nsEventStateManager::MaybeDispatchMouseEventToIframe():

+  // Check to see if we're an IFRAME and if so dispatch the given event
+  // (mouseover / mouseout) to the IFRAME element above us.  This will result
+  // in over-out-over combo to the IFRAME but as long as IFRAMEs are native
+  // windows this will serve as a workaround to maintain IFRAME mouseover
state.
+  nsCOMPtr<nsIDocument> parentDoc;
+  //If this is the first event in this window then mDocument might not be set
yet.
+  //Call EnsureDocument to set it.

Add space after // here, and in all other code you're touching too. Bee
consistent.

+      docContent->GetTag(*getter_AddRefs(tag));
+      if (tag == nsHTMLAtoms::iframe) {

While you're moving this code around, wanna make it XML safe too? Add
docContent->IsContentOfType(nsIContent::eHTML) after the atom check.

- In nsEventStateManager::GenerateMouseEnterExit():

+	 // Yes, this is weak, but it will stick around for this short time :)
+	 nsIContent* temp = targetElement.get();

Loose the .get() while you're at it...

+      if (mLastMouseOverElement != mFirstMouseOutEventElement
+	   || !mFirstMouseOutEventElement) {

Move the or operator to the end of the first line for consistency with the rest
of the code here.

Other than those nits, this looks good to me.

Comment 31

[John Keiser (jkeiser)]

Fix checked in.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. i'm not so sure that this fix was correct. I know that i in some other bug
(or it might have been a w3c post) said that we should only target mouse-events
to elements. However it was pointed out to me that the spec said "element"
rather then "<code>Element</code>" which apparently ment that the event could be
targetted at any node.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

found it, it was bug 42717

Comment 34

[John Keiser (jkeiser)]

Point well taken, but now we're completely at odds with *compatibility* on pages
that do dynamic menus, and there are a goodly number of them.  mousemove, FYI,
still gets targeted to textnodes.  Firing mouseout on textnodes can cause cancer.

File a bug and we can duke it out there.  A huge portion of this patch should
stay in the tree anyway, no matter what decision is made.

Comment 35

[John Keiser (jkeiser)]

This caused crash regression bug 185850.

Comment 36

[José Jeria]

Can this fix anyhow affect tooltips? Cause tooltips dont work as expected
anymore. (bug 185965).

Comment 37

[José Jeria]

Please also take a look at this regression that causes a crash. Bug 186132.

Comment 38

[Yves Lambert]

testcase test one don't work

Comment 39

[Yves Lambert]

testcase id=52001 test one doesn't work: the hover opens 2 alert boxes instead of 1

Comment 40

[John Keiser (jkeiser)]

Reopening.  Patch upcoming.  I regressed this with the last-minute change in bug
185889.

Comment 41

[John Keiser (jkeiser)]

Attachment: Patch, again

OK, this restores the regressed behavior.  The reason this regressed is, we now
target events against the *content* for the parent element but the *frame*
remains the same.  So we have to once again search the parent elements of the
frame here.  The alternative is to be able to access the content the event will
be targeted against (a good idea) but that is not a 1.3-ish thing.

Comment 42

[John Keiser (jkeiser)]

Comment on attachment 113141 [details] [diff] [review]
Patch, again

Given that this is simply restoring old code, asking for both r/sr at the same
time.

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 113141 [details] [diff] [review]
Patch, again

sr=jst

Comment 44

[John Keiser (jkeiser)]

Comment on attachment 113141 [details] [diff] [review]
Patch, again

*sigh* I suck.	Testing the world reveals that this in fact re-breaks tooltips.

Comment 45

[Nathan Kurz]

Attachment: another testcase for mouseover/mouseout

Here's another testcase that might help.

Comment 46

[John Keiser (jkeiser)]

Fix checked in.  Much better chance of it sticking this time.

Comment 47

[Boris Zbarsky [:bzbarsky]]

This broke dragging of text -- see bug 193568

Comment 48

[R.K.Aa.]

but it may have fixed bug 193104 and bug 193321

Comment 49

[Paul Wyskoczka]

Marking verified (rs). Checked in a while ago.