3.54 KB, text/html
3.54 KB, text/html
2.87 KB, patch
Brian Ryner (not reading): review+
|Details | Diff | Splinter Review|
2.42 KB, text/html
5.99 KB, patch
saari (gone): review+
Brian Ryner (not reading): superreview+
|Details | Diff | Splinter Review|
2.15 KB, patch
|Details | Diff | Splinter Review|
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?
jkeiser wanted this
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.
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.
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
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.
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...
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.
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.
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.
Created attachment 109652 [details] illustration 1 - nested divs with #text nodes as first and last child
Created attachment 109653 [details] illustration 2 - nested divs with #text nodes as last child 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.
my comments about bugs are not appropriate for the trunk. sorry.
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.
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.
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*.
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.
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.
imagemaps are bug 110072, a separate problem.
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.
Comment on attachment 111593 [details] [diff] [review] Patch sr=jst
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 :)
If you're looking for things this may break, be sure to run the DIG tests
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.
Fix checked in.
Created attachment 113326 [details] Test case for textnode events 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?
> 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.
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.
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.
Created attachment 114042 [details] [diff] [review] Patch #2 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.
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.
Comment on attachment 114042 [details] [diff] [review] Patch #2 Looks ok to me. Fix the hard tab in PresShell::GetEventTargetContent.
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.
Comment on attachment 114042 [details] [diff] [review] Patch #2 r=saari
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).
Fix checked in. Much better chance of it sticking this time.
Created attachment 114945 [details] [diff] [review] Backout Patch 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.
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.
Adding 193799 as a blocker for this bug, though it is not yet ascertained for certain that that is caused by this fix.
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).