Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Description • 22 years ago

Bug 103055 made it so that we don't fire mouseout and mouseover at textnodes.
This might not fully be according to spec, see bug 42717 comment 7.

We do however fire mousemove at textnodes, which might be inconsistent if we
decide to keep this new behaviour of not targetting textnodes for other mouseevents.


As far as i understood bug 103055 a lot of sites failed because they got an
unexpected mouseout when the mouse was moved from being over the element to
being over the textnode.
However it seems to me that the problem isn't firing the event at the textnode,
but rather that there was an mouseout event fired at the element in the process. 

Consider a mouse moving along the dotted line

          :
 +--------:--------+
 |    my b:tton    |
 +--------:--------+
          :

This should IMHO fire the following events:

mouseover  target: element
mouseover  target: textnode
mouseout   target: textnode
mouseout   target: element

i.e. a mouseout isn't fired at the element until the mouse is actually moved out
from the elements bounds.

Shouldn't this make most sites behave good?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 1 • 22 years ago

jkeiser wanted this

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 2 • 22 years ago

Hmm.. i just realized that it might get hairy if you have something like:

          :
+--+--+---:---+--+--+   <- T0
|a |b |c  :   |  |  |
|  |  +---:---+  |  |   <- T1
|  |      :      |  |
|  +------:------+  |   <- T2
|         :         |
+---------:---------+   <- T3
          :
          \/

what should we fire an event on when entering the boxes? The easiest thing would
probably be to only fire something like

mouseover  target: c   (at T0)
mouseout   target: c   (at T1)
mouseover  target: b   (at T1)
mouseout   target: b   (at T2)
mouseover  target: a   (at T2)
mouseout   target: a   (at T3)

However this will make it very hard for developers to figure out when the 'a'
box is entered and left, which is defenetly something that they would want to do.

So instead we could fire 

mouseover  target: a   (at T0)
mouseover  target: b   (at T0)
mouseover  target: c   (at T0)
mouseout   target: c   (at T1)
mouseout   target: b   (at T2)
mouseout   target: a   (at T3)

Since that would allow developers to only listen to events targeted at 'a' to
figure out when it is entered and left.

However firing 3 bubbling events when the top border is crossed is probably bad
performance-wise, we will end up firing a lot of bubbling events as the mouse
moves across a page.

	John Keiser (jkeiser) Assignee
	Comment 3 • 22 years ago

I like the proposal, and it makes more sense than the existing mouseover /
mouseout behavior.  But given that problem, this would only really work if you
make onmouseover / onmouseout a non-bubbling event.  I think that would solve
all problems *and* match developer expectations.  The unfortunate part is we'd
be going out on a real limb: it would go against both the spec and IE.

Of course, developers *could* just check the target of the event to see if it is
the current element to determine whether mouseover / mouseout happened on
*their* element instead of on a child element, but none of them will do that or
really expect it to happen--violates the Principle of Least Surprise.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 4 • 22 years ago

in any case i think we should be consistent with wether we fire mouse-events on
textnodes or not, if we don't fire mousemove/out we shouldn't IMHO fire mousemove

	John Keiser (jkeiser) Assignee
	Comment 5 • 22 years ago

I think the upshot of all this is, we're probably stuck with bubbling the
events, and if we're stuck with that, 

1. Should we fire mouseover / mouseout events on textnodes?
IF there is a good reason to be able to do this, we should consider it.  Due to
web compatibility, though, we're really looking at a standards mode rendering
thing.  I hate hate hate quirks.  It should die.  But if there is a real
struggle between web compatibility and future web application designers (and I'm
not so sure about that yet) then we may do it.  Standards mode becomes less of a
thing people write web pages in, and more of a thing people write applications
in.  I can live with that.

2. OK, so do we fire mousemove events at text content?  sicking is probably
right in that we should not fire mousemove events at something if we have not
mouseover'd or mouseout'd it.  If we answer "no," I see no major impact.  Can
anyone think of such an impact?

It can be argued, of course, that mouseover and mouseout are not harbringers of
the mousemove event, but are in fact the guardians of :hover.  In that case, it
would make sense for them to be associated with :hover (which can only matter on
an element).  mousemove could be considered a different beast.

But if one were to advance such an argument, one might be accused of making up
dubious arguments so one does not have to introduce potentially destabilizing
code into the application.

	Vladimir Ermakov
	Comment 6 • 22 years ago

A reason for keeping mousemove could be that if someone does want to know
whether the mouse is over a particular text node, they can use mousemove as an
alternative to mouseover/mouseout... If mouse move has fired on text node then
the mouse is over it, if it fires on another node then the mouse has come out...

	Doron Rosenberg (IBM)
	Comment 7 • 22 years ago

From a actual webdevelopment standpoint, the bug was causing issues with dhtml
menus - you had div with text (a menu) and the div and onmouseout="hideMe()".
which wasc alled when you moused over the text node inside the div.

I think that having a different event model in quirks and standards mode for
text nodes is bad, it will just confuse rather than help developers.

	Bob Clary [:bc] (inactive)
	Comment 8 • 22 years ago

The prose in
http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events-eventgroupings-mouseevents
was inspired by HTML 4 and DOM 0 browsers and repeatedly refers to mouse events
and Elements not mouse events and text nodes. Regardless of ekrock's comment I
do not see an errata in http://www.w3.org/2000/11/DOM-Level-2-errata related to
mouse events, Element and text nodes.

This was a disagreement we have had from the beginning on this subject. I don't
think the standard implies that mouse events have meaning for text nodes but
only for Elements.

I also do not think that we should change event handling based on quirks vs
standards mode.

If we have decided to not fire mouseout/mouseover for text nodes, then I do not
think that we should be firing mousemove either.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 9 • 22 years ago

yeah, i think we all agree that differences between quirks and standards would
be a unfortunate way to go.

The more i think about it the more i think that firing mouse-events at textnodes
is a bad idea. I can't think of a usecase where you would want to know which
textnode the mouse is over.

What you could possibly want is to know what *char* the mouse is over, but that
is obviously not covered by the current spec. I think that would best be
implemented using some function that could map either an event to a character,
or an X/Y pair to a character. And then let the user use the existing events.


However the question raised in comment 2 is still an issue. I would think it
would be great if we could fire the second event-list if that

a) doesn't slow us down, and
b) doesn't break a lot of sites.

I'm not too worried about b) since i think that in most cases people attach
mouse eventhandlers to leaf elements. But a) might defenetly be a problem.

	Bob Clary [:bc] (inactive)
	Comment 11 • 22 years ago

Illustration 2 is what Jonas was referring to in comment #2.

This should let us look at what we are doing now and test any changes to see if
they meet with expectations.

Note that at target, mouse events on #text nodes do not have a currentTarget
which is a bug and that depending on how fast you move the mouse you may or may
not get events on DIVs containing the #text nodes which is also a bug.

	Bob Clary [:bc] (inactive)
	Comment 12 • 22 years ago

my comments about bugs are not appropriate for the trunk. sorry.

	Vladimir Ermakov
	Comment 13 • 22 years ago

I dont think firing any events on a text node can be of any use. I think we can
get rid of them altogether. Click for example... should not be fired as well.

	John Keiser (jkeiser) Assignee
	Comment 14 • 22 years ago

Through numerous conversations it appears that not firing events at textnodes is
where we will end up.  To clarify: if you directly fire an event at the textnode
*content* (like a mutation event) you are fine.  But if the system determines
the node based on the x,y position of the mouse, it will only target at textnodes.

Suggestions or potential pitfalls welcome.  This is scary :)  But no one in
dynamic interfaces so far has a problem with it.

	John Keiser (jkeiser) Assignee
	Comment 15 • 22 years ago

Last sentence of that first paragraph should read:

But if the system determines the node based on the x,y position of the mouse, it
will only target at *elements*.

	John Keiser (jkeiser) Assignee
	Comment 16 • 22 years ago

This is not perfectly dependent on bug 148542, but the patch there makes me more
confident in this patch.  I have made the patch to do this (do not fire mousey
events at textnodes), and it fixes tooltips but breaks selection, so I have to
do a little investigation and see what's involved.

	Carl Friedrich Spilcke-Liss
	Comment 17 • 22 years ago

I found the problem also with images/imagemap using BuildID 2003010205. If you
look at http://www.spiegel.de the navigation bar at the left consists of an
image map with no gaps. The current position is indicated by a small triangle
left to your position.

If your mouse is enters the navigation area, the menu point where you enter is
also indicated by the triangle. Leaving the navigation area without clicking
correctly results in an onmouseout and thus the triangle is hidden again. But if
you go yith your mouse to a third navigation entry the onmouseout is ignored. I
expected the new entry under the mouse to be marked by the triangle, but I found
the entry point in the navigation area still marked by the triangle.

	John Keiser (jkeiser) Assignee
	Comment 18 • 22 years ago

imagemaps are bug 110072, a separate problem.

	John Keiser (jkeiser) Assignee
	Comment 20 • 22 years ago

OK, this patch fixes the tooltips problem in bug 185965 and doesn't regress
anything I have found.  Although I am sure it *will* regress something.

What this does is not fire GUI mouse events at textnodes, period.

	Johnny Stenback (:jst)
	Comment 21 • 22 years ago

Comment on attachment 111593 [details] [diff] [review]
Patch

sr=jst

	John Keiser (jkeiser) Assignee
	Comment 22 • 22 years ago

Note that this will affect the bug 148542 fix, in that it will need to be
incorporated.  That is actually why I wrote that fix, it just hasn't gone in
soon enough to build on it.  This fix looks prettier with that fix in :)

	saari (gone)
	Comment 23 • 22 years ago

If you're looking for things this may break, be sure to run the DIG tests

	John Keiser (jkeiser) Assignee
	Comment 24 • 22 years ago

I couldn't find the DIG tests specifically, but I found some of their demos,
specifically relating to hover and stuff like that, and it all still works with
this patch.

	John Keiser (jkeiser) Assignee
	Comment 25 • 22 years ago

Fix checked in.

	Nathan Kurz
	Comment 26 • 22 years ago

I just recompiled to include this code, and it doesn't seem to be doing
anything for me.  Here's a test case to show what I'm still seeing with a Feb 1
2003 build of Phoenix:	mouseover/mouseout events for the transitions to/from a
container to the anonymous text within (as well as lots of missed events for
the container).  Is there a more official test case that I could try to see if
this (somehow) a different bug?

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 22 years ago

> anonymous text within

Whatever we do, please let's not call text nodes anonymous.  They are not.  The
term "anonymous node" actually means something, and does _not_ apply to the text
nodes in question.

That said, I'm also seeing the problem with that last testcase with build
2003-01-30-08 on Linux.  Also, the other two testcases attached in this bug also
fail in this build.

So I'm reopening this, since it is in fact not fixed.

	John Keiser (jkeiser) Assignee
	Comment 28 • 22 years ago

Look more closely at the testcases ... the event is actually not being fired at
the textnode, as you can see it never actually fires the handler.  However,
someone is setting the *target* in there, which is the source of both this and
bug 103055.

The most recent testcase is bug 103055; but that will be fixed if I can figure
out who is re-setting my precious target.

	John Keiser (jkeiser) Assignee
	Comment 29 • 22 years ago

To clarify, yes, mouseover / mouseout events are actually being fired at text
content (bug 103055), but other mouse events are not; the target is just being
reported wrong.  This bug isn't awfully noisy and this is a related issue so
I'll just fix it here.

	John Keiser (jkeiser) Assignee
	Comment 30 • 22 years ago

This is it!  This makes tooltips work properly, unexpectedly fixes the image
map mouseover/mouseout bug, and makes click work like
mouseup/mousedown/mouseover/etc.  selection still works fine, and I submitted
this with the patch.

The problem was that GetEventTargetContent(), which was called by GetTarget(),
grabbed the frame instead of the content.  The underlying problem IMO is that
we store the current frame and content *externally from the event*, but that is
a bigger fix than we want here.

Besides that, I made click event dispatch and mouseover dispatch use
GetEventTargetContent() to get the content for the new event.

	John Keiser (jkeiser) Assignee
	Comment 31 • 22 years ago

Comment on attachment 114042 [details] [diff] [review]
Patch #2

Asking for bryner-sr first, will ask for r=saari afterwards--bryner has been
more involved in this mouse stuff.

	Brian Ryner (not reading)
	Comment 32 • 22 years ago

Comment on attachment 114042 [details] [diff] [review]
Patch #2

Looks ok to me.  Fix the hard tab in PresShell::GetEventTargetContent.

	saari (gone)
	Comment 33 • 22 years ago

I'm a little worried that this optimizes out 
-        nsFrameState state;
-        mCurrentTarget->GetFrameState(&state);
-        state |= NS_FRAME_EXTERNAL_REFERENCE;
-        mCurrentTarget->SetFrameState(state);

and GetCurrentEventFrame doesn't seem to cover it. Finding the bugs where we
crash because frames have gone away during an event is a pain.

	saari (gone)
	Comment 34 • 22 years ago

Comment on attachment 114042 [details] [diff] [review]
Patch #2

r=saari

	John Keiser (jkeiser) Assignee
	Comment 35 • 22 years ago

Comment on attachment 114042 [details] [diff] [review]
Patch #2

Asking for 1.3b approval.  I think this is important.  This fixes a number of
regression I introduced a couple of weeks ago (and fixes a few unexpected
bugs).	See the dependent bugs for more information on what gets fixed (a
topcrasher and a topembed+ are among them).

	John Keiser (jkeiser) Assignee
	Comment 36 • 22 years ago

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

	John Keiser (jkeiser) Assignee
	Comment 37 • 22 years ago

Here is the patch necessary to back out retargeting.  It re-fixes the regressed
stuff and deliberately re-breaks this bug and bug 103055.

One day perhaps it will be possible to fix everything at once.	We're certainly
closer now.

	John Keiser (jkeiser) Assignee
	Comment 38 • 22 years ago

Comment on attachment 114945 [details] [diff] [review]
Backout Patch

Asking for r=/sr= to speed the process but DO NOT CHECK THIS IN unless you have
tested that it doesn't re-regress unexpected things.  The browser is chugging
along fine but I haven't checked all the dependent bugs yet.

	John Keiser (jkeiser) Assignee
	Comment 39 • 22 years ago

Adding 193799 as a blocker for this bug, though it is not yet ascertained for
certain that that is caused by this fix.

	John Keiser (jkeiser) Assignee
	Comment 40 • 22 years ago

Comment on attachment 114945 [details] [diff] [review]
Backout Patch

un-asking, I think we'll be able to clear the remaining blockers in time
(patches exist for 2 of them).