Closed Bug 103055 Opened 23 years ago Closed 22 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.3beta

People

(Reporter: chrisbolt, Assigned: john)

References

()

Details

(Keywords: testcase, topembed+, Whiteboard: [adt2] [ETA Needed][FIX])

Attachments

(8 files, 3 obsolete files)

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.
Attached file Test case (deleted) —
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?
I have a nagging feeling that this is a dupe, but I could be wrong. I definitely think the background problem is new, however.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
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?
Status: NEW → ASSIGNED
Keywords: testcase
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.
We have a new select guru, and it's not me ;-) John is this fixed with that patch of yours? :-)
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.
Target Milestone: --- → mozilla1.1
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.
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 :-)
"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.
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?
Attached file bell.ca testcase (deleted) —
*** Bug 134415 has been marked as a duplicate of this bug. ***
OS: Windows 2000 → All
Hardware: PC → All
Keywords: testcase
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
nominating for topembed
Keywords: topembed
Attached file yet another testcase (really small!) (deleted) —
This testcase is a really simple one - hovering over the text causes an alert it shouldn't be causing.
Keywords: topembedtopembed+
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.
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.
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).
Attached file testcase with style change (obsolete) (deleted) —
This is not just with alert(). Attached testcase makes a change to the style, and this also occurs before the mouse leaves the td.
Attached file Test case with style change (deleted) —
Whoops, testcase was broken. This should work. Tested here with 2002082608 incidentally.
Attachment #96751 - Attachment is obsolete: true
*** Bug 169461 has been marked as a duplicate of this bug. ***
->bryner, high priority
Assignee: joki → bryner
Status: ASSIGNED → NEW
Target Milestone: mozilla1.1alpha → mozilla1.3alpha
Blocks: 178882
Severity: normal → major
Keywords: nsbeta1+
Priority: -- → P1
Whiteboard: [adt2] [ETA Needed]
Attached file explicit testcase (deleted) —
This bug begins to be really critical. Please help us brave moz developers
I have a fix, reassigning.
Assignee: bryner → jkeiser
Attached patch Patch (obsolete) (deleted) — Splinter Review
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.
Status: NEW → ASSIGNED
Whiteboard: [adt2] [ETA Needed] → [adt2] [ETA Needed][FIX]
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Attached patch Patch v1.1 (deleted) — Splinter Review
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.
Attachment #108466 - Attachment is obsolete: true
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 :)
Attachment #108696 - Flags: review?(bryner)
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.
Attachment #108696 - Flags: review?(bryner) → review+
Attachment #108696 - Flags: superreview?(dbaron)
Attachment #108696 - Flags: superreview?(dbaron) → superreview?(jst)
Blocks: 110072
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.
Attachment #108696 - Flags: superreview?(jst) → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
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.
Blocks: 130620
Depends on: 185850
This caused crash regression bug 185850.
Can this fix anyhow affect tooltips? Cause tooltips dont work as expected anymore. (bug 185965).
Blocks: 185965
Please also take a look at this regression that causes a crash. Bug 186132.
Blocks: 186500
testcase test one don't work
testcase id=52001 test one doesn't work: the hover opens 2 alert boxes instead of 1
Depends on: 186132
Reopening. Patch upcoming. I regressed this with the last-minute change in bug 185889.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch, again (obsolete) (deleted) — Splinter Review
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 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.
Attachment #113141 - Flags: superreview?(jst)
Attachment #113141 - Flags: review?(bryner)
Comment on attachment 113141 [details] [diff] [review] Patch, again sr=jst
Attachment #113141 - Flags: superreview?(jst) → superreview+
Comment on attachment 113141 [details] [diff] [review] Patch, again *sigh* I suck. Testing the world reveals that this in fact re-breaks tooltips.
Attachment #113141 - Attachment is obsolete: true
Attachment #113141 - Flags: superreview+
Attachment #113141 - Flags: review?(bryner)
Blocks: 124789
Here's another testcase that might help.
Depends on: 185889
Fix checked in. Much better chance of it sticking this time.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
This broke dragging of text -- see bug 193568
but it may have fixed bug 193104 and bug 193321
Marking verified (rs). Checked in a while ago.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: