Steps: Hover the mouse over an image and notice the cursor changes to hand with index finger pointing. Now hover just above the image and notice the cursor changes back to an 'I' Now (without moving the mouse) scroll down with the down arrow key until the picture is under the cursor. The cursor should change to a hand but it doesn't. It remains an 'I' Other Browsers: The cursor changes to handshape in all browsers except Safari. Webkit nightly tested: r29807
IIRC this same issue affects scrolling with the space bar as well.
In Mac Safari, the mouse cursor is hidden when scrolling - perhaps we should do the same on Windows?
*** Bug 17959 has been marked as a duplicate of this bug. ***
The cursor, the link target displayed in the status area, and the hover state of elements on the page are all affected (or should I say unaffected), and the problem also occurs with scrolling via the mousewheel. Hiding the cursor while scrolling would not be consistent with Windows platform conventions or other browsers on Windows. Instead we should probably just make sure the same sort of updates that happen after a mouse move event happen after a scroll event. (I suggest changing the bug summary to "Cursor/link target/hover state not updated during scroll", which I don't have the power to do.)
Hi, I'm preparing a fix for this. Yuzo
Created attachment 31772 [details] A proposed patch for this bug
Comment on attachment 31772 [details] A proposed patch for this bug Typo updateCurrentMousePoistions should be updateCurrentMousePositions I'm also really unhappy with the the addition of multiple #if PLATFORM(WIN) cases. There is no explanation of why these should be windows only. Sending a mouseMove event in response to a key board even t seems highly suspect and i'd want tests for that behaviour, and confirmation that other browsers do it I don't think we should be refreshing the cursor on every key event if we can avoid it -- in the general case we won't need to update, so updating every time seems incorrect. I would think a superior solution would be to update the mouse state if an element that is currently under the mouse scrolls, but i think hyatt would be the one to talk about that in general.
Created attachment 31778 [details] A revised patch. Hi, Thank you for your review. I've addressed your comments. Can you take another look? Yuzo
Comment on attachment 31778 [details] A revised patch. Conflict marker: +>>>>>>> .r45079 resolve-ChangeLogs script will fix ChangeLogs when you get conflicts updating. That way you wont have to do it manually (and thus worry about leaving conflict markers). Style: +void EventHandler::updateMousePositions(const PlatformMouseEvent& mouseEvent) { "view" is better than "v": + FrameView* v = m_frame->view(); Style: +void EventHandler::recordScrollPosition() { Style: (braces): +void EventHandler::simulateMouseMoveIfNecessary() { In general this looked fine, besides the nits. I'll need to look again for content.
Created attachment 31822 [details] Addressed comments Hi, Thank you for the review. I've: - Removed the conflict mark - Renamed v view - Fixed braces starting functions Can you take another look? Yuzo
(In reply to comment #10) > Created an attachment (id=31822) [review] > Addressed comments > > Hi, Thank you for the review. > > I've: > - Removed the conflict mark > - Renamed v view > - Fixed braces starting functions > > Can you take another look? > > Yuzo > Ping?
Mac does not seem to have this bug.
How does Mac avoid needing this code?
Comment on attachment 31822 [details] Addressed comments Ok. What I need from you to r+ this patch, is I need explanation as to why this is the correct fix. The fix itself looks fine, but why is this the right way to fix this? Why does Mac not need this code for example? Try the example on a Mac with a scroll wheel mouse and you'll see cursor updates work as expected.
Also, I will note that your current solution has observable side-effects. JavaScript could detect your simulated mouse move events using onmousemove handlers.
Thank you for your review. onmousemove side effect is not good. I need to fix it. As to why Mac doesn't need this change: As far as I tested, Mac OS always sends a mouse move event after it sends a mouse wheel event. (So actually for Mac I should disable the simulated event that this patch will introduce.) As for scroll by key event, on Mac the mouse cursor is hidden when a key is pressed. Hence no need to update its shape. I'll - find a way to avoid onmousemove event - disable the simulated event for mac - add comments that explain why Mac works fine without this patch. and get back to you. Yuzo (In reply to comment #15) > Also, I will note that your current solution has observable side-effects. > JavaScript could detect your simulated mouse move events using onmousemove > handlers. > (In reply to comment #14) > (From update of attachment 31822 [details] [review]) > Ok. What I need from you to r+ this patch, is I need explanation as to why > this is the correct fix. The fix itself looks fine, but why is this the right > way to fix this? Why does Mac not need this code for example? Try the example > on a Mac with a scroll wheel mouse and you'll see cursor updates work as > expected. >
Created attachment 32291 [details] Added rationale. Hi, Eric, Thank you for the review. Can you take yet another look? On second thought, I come to believe that we should actually cause mousemove event after scrolling via wheel/keys because - MAC sends a mousemove event anyway, and - there seem to be no other easy ways to cause mouseover or mouseout events. I've added a comment to explain the above rationale and why MAC doesn't need this patch. Yuzo
Comment on attachment 32291 [details] Added rationale. This patch will fix a Chromium bug if PLATFORM(CHROMIUM) is added to this line: 2419 #if PLATFORM(WIN) || PLATFORM(GTK) The bug question is http://crbug.com/15656, which is Mac-specific, but we do need this for our other platforms as well.
Created attachment 32408 [details] Add CHROMIUM to the target platform. Mark, Thank you for your comment. I've added PLATFORM(CHROMIUM) to the #if. Yuzo
Ping?
Comment on attachment 32408 [details] Add CHROMIUM to the target platform. > + const bool result = handleWheelEventInternal(e); There are many local variables that could be marked const. Normally it is not our style in the WebKit project to do this just because the variable is not modified in the function. It seems an unnecessary stylistic difference in your new code. > + // For Windows and GTK, we need to simulate a mousemove event after scrolling by mouse wheel or up/down/space key. > + // For MAC, we don't need to, because the OS sends a mousemove event after a mousewheel event, and hides the mouse cursor while scrolling by keys. > + // TODO: Examine other platforms. The name of the platforms are Windows and Mac. Not Windows and MAC. This comment seems incorrect to me. An almost identical issue exists on the Mac. I think the issue is that the Mac has code in WebKit to handle this, and the other platforms don't. I don't think the difference is in the OS. You should patch the platforms to be the same instead of creating an unnecessary platform difference. Even if what you said was true about the wheel events, it's definitely not true about keyboard events. In fact, by leaving this code turned off for Mac you miss the opportunity to fix a bug I've seen where scrolling with arrow keys on Mac doesn't result in update of hover. Worse, by including PLATFORM(CHROMIUM), you'd fix the Mac version of Chrome but leave this broken in the Mac version of Safari. > + // Simulating a mousemove event has a side effect that onmousemove handler is invoked when the mouse doesn't actually move. Tiny language nitpick. Referring to the "onmousemove" handler is unnecessarily inaccurate. Setting an onmousemove attribute is only one of many ways to set up a handler for a mousemove event, and not one that needs to be emphasized in comments. > + bool keyEventInternal(const PlatformKeyboardEvent&); > + bool handleWheelEventInternal(PlatformWheelEvent&); Why does one of these functions have "handle" in its name, but not the other? I guess because they reflect the names of the original functions that you broke into two pieces. It's a little ugly to just add a level of function call so you can wrap the function in new code. If people find other similar requirements and go on repeating this pattern, then we will end up with Internal2, Internal3, versions of these functions. The names do nothing to tell programmers what should be in the outer and what should be in the inner version. I don't think it's a good design pattern for the long term, although I think it's OK here. It's strange that we will record scroll positions and simulate mouse move events even for keyboard events we choose not to handle. I think this patch really isn't tackling this at the right level. It's true that this is the easiest level to patch for someone who's not familiar with the code, but scrolling can also be triggered by scripts rather than by events, and the scripts could run based on a timer. It's not right to trigger the simulated events based on the real events. Instead I think the simulated mouse move events should be triggered more directly by the actual document scrolling. A good test case would be a timer that scrolls. The cursor should be updated each time the scroll happens even if there is not keyboard or mouse wheel events. > + void updateMousePositions(const PlatformMouseEvent& mouseEvent); We do not give names to arguments like mouseEvent in function declarations when the type already makes the argument's purpose clear. > + const IntPoint& globalPos() const { return m_globalPosition; } I know this is just following the standard of the original code, but I think globalPosition() would be a better name for this new function. I think it's unnecessary to have the simulated event have so much incorrect state, such as false for all the modifier keys. Instead we should make the simulated event reflect the real state as much as possible. I also think that the simulated event should not contain the same mouse position as the last real event. The mapping from a global position to a local one can be affected by things such as the window position, and that could have changed since the last real mouse event, for example if a script moved the window. The idea of this code is great, but I think it needs more work and to be less platform-specific.
This is not platform-specific. It happens on all platforms.
I thought further about the best way to address this issue. It seems to me that scrolling and elements moving is made real when we do layout, so we may want to hook tis detection into the layout timer and layout machinery. After layout we can check if the current mouse position is still over the same thing it was over before, and if not send the appropriate events. We'll have to check that layout covers all the scrolling cases. If not, then we can have a separate set of the same kind of code for scrolling. Moving windows is yet another similar case. In all cases, it seems having a record of where we previously reported the mouse over is helpful so we can see if the new location is different in any way. I'm thinking that we might want to keep the actual DOM event that was sent to see if it's at all different. Besides the DOM events, we're also interested in making sure the cursor has the correct shape in these various cases. One simple model for this might be to have a recheckMousePositionSoon function on EventHandler that could be called in all places where there was a change that might require re-communicating the mouse position to the web page. I don't think that a check on the way in and the way back out of event handlers is the right bottleneck for this. I think it's the actual changes to rendering, scrolling, and window position that are apropros.
Thank you very much for the very careful and detailed review. I agree. The piece of code that scrolls the document should be responsible for triggering update for mouse cursor/hover/link status. My band-aid like approach could be worse for the project in a long run. Probably surgery is called for here. :) Yuzo
Darin, Safari/Mac appears to generate extra mouse move events for lots of things (including, e.g., mouse downs/ups), thereby avoiding a host of bugs. When scrolling using the keyboard, in Safari/Mac, the cursor (and tooltip) disappear, thereby eliminating a bunch of problems; actually, now that Chromium/Mac also makes the cursor (and consequently the tooltip, apparently) disappear, those same problems go away. Now, unfortunately, the mouse moving code (EventHandler::mouseMoved()/handleMouseMoveEvent()) are very tightly coupled with PlatformMouseEvent. To handle "virtual mouse moves", we need to at least do either (1) send fake mouse move events (ugly, but expedient) or (2) decouple the aforementioned code so that it can handle such "virtual mouse moves" without a PlatformMouseEvent (make an "EventHandler::mouseMovedVirtually()" method or some such). In some sense, the difference between (1) and (2) is cosmetic. You could decouple so far as to push a lot of the logic down into the layout, but that would be drastic and you'd have to make the layout aware of the mouse position. My first instinct to fix the scrolling-related issues is to hook up (1) or (2) in FrameView::valueChanged() (probably inside the "if (offset != scrollOffset())"). Similarly, you could do the same in FrameView::performPostLayoutTasks() -- I think this would probably handle any possible resize-related issues. (There are also possible window-movement issues -- i.e., the window moves without the mouse moving -- though I don't know offhand where that would get hooked up.) Hooking up a fake mouse event is expedient, if a little repugnant; decoupling the code is a bit fragile and goes pretty deep. (E.g., on resize, the mouse may end up over a scrollbar, so you can't just ignore the scrollbars.) Maybe I'll come up with a patch which sends a fake mouse event, and see if it works.
Created attachment 34540 [details] Quick and dirty patch, as per my comments. For commentary only; no changelog entries. Only tested on Chromium/Mac. Hmm. The effect seems to be different from what happens in Safari/Mac (not for scrolling with arrow keys, but with trackpad/mousewheel -- see <http://crbug.com/15656>).
There are at least three distinct related questions in my opinion: a) How we detect that there's a change that requires some mouse position processing. There are many different types of changes that could trigger the need for this, such as an element moving that has a CSS cursor associated with it so that it's no longer under the mouse, any kind of scrolling, or even a change to the CSS cursor property without any geometry change at all. We need a good architecture so that the new processing gets done. I don't think it necessarily has to be done right away. We could use a zero duration or small duration timer to coalesce these so that multiple changes don't require multiple attempts to process things, and so that the processing can happen after layout has occurred. In fact, when it comes to status bar messages, even changes in the keyboard state are relevant. For example, if you press the option key on the Mac, we need to recompute the status message. I believe this is currently done by simulating a "mousemove" when there is change in the state of modifier keys, done in the Mac-specific WebKit code. b) What should be seen at the DOM level. For example, if an element moved and that changes the cursor, then presumably ideally we don't want DOM event handlers to see a DOM "mousemove" event since the mouse didn't move. Or maybe we do, but if so it's an intentional choice, not just a side effect of how our code works internally. We might be willing to live with an excess "mousemove" event, but it's probably not working as designed if we send one. On the other hand, if an element moves so it's no longer under the mouse, we probably *do* want to send that object a "mouseout" event even if we would not have sent a "mousemove" event. What do we want seen at the DOM level when the option key goes down? Probably not a "mouse move" event, so that's a minor bug on the Mac that would be nice to fix. Unless we decide it's a feature rather than a bug. c) Once we detect a change, how we trigger the additional processing. It may be expedient to use what looks like a "mousemove" event to minimize refactoring of the existing code. But I think that sort of lack of clarity and logic in the code is unnecessary. We should be willing to make some changes to make this logical unless it's so difficult that it makes the problem a lot harder to fix. (In reply to comment #25) > Safari/Mac appears to generate extra mouse move events for lots of things > (including, e.g., mouse downs/ups), thereby avoiding a host of bugs. It would be very useful to determine where those events come from. Is it the Mac OS X AppKit that is providing these events? Or is it the Mac-specific WebKit code that creates them? If the former, then we should just live with them as is. If it's the latter, maybe we can solve this in a platform independent way and get rid of the unneeded code. > When > scrolling using the keyboard, in Safari/Mac, the cursor (and tooltip) > disappear, thereby eliminating a bunch of problems; actually, now that > Chromium/Mac also makes the cursor (and consequently the tooltip, apparently) > disappear, those same problems go away. This is an interesting point. It hides some problems (cursor and tooltip) but doesn't affect others at all (status bar, DOM events as seen on the web page, hover effects in the page). Why don't other platforms have this behavior? Is this really a unique feature of Mac OS X, or is this something we'd want on Windows and Unix desktop systems too? > Now, unfortunately, the mouse moving code > (EventHandler::mouseMoved()/handleMouseMoveEvent()) are very tightly coupled > with PlatformMouseEvent. To handle "virtual mouse moves", we need to at least > do either (1) send fake mouse move events (ugly, but expedient) or (2) decouple > the aforementioned code so that it can handle such "virtual mouse moves" > without a PlatformMouseEvent (make an "EventHandler::mouseMovedVirtually()" > method or some such). In some sense, the difference between (1) and (2) is > cosmetic. You could decouple so far as to push a lot of the logic down into the > layout, but that would be drastic and you'd have to make the layout aware of > the mouse position. You're talking about my issue (c) here. But I think that my issue (b) also comes up. Do we want actual mouse moves to show up in the DOM or not? I don't think this refactoring and clean-up is particularly difficult, and I'd love to see it done for code clarity either before or after this bug is fixed. I agree that it doesn't have to be done to fix the bug. > My first instinct to fix the scrolling-related issues is to hook up (1) or (2) > in FrameView::valueChanged() (probably inside the "if (offset != > scrollOffset())"). Similarly, you could do the same in > FrameView::performPostLayoutTasks() -- I think this would probably handle any > possible resize-related issues. (There are also possible window-movement issues > -- i.e., the window moves without the mouse moving -- though I don't know > offhand where that would get hooked up.) This seems to cover only a tiny fraction of problem (a). It's true that a FrameView scrolling can move an element. But also, changes to the element's geometry can move it. And overflow area scrolling can also move it. I'd like to understand our general strategy to detecting when elements move out from under the mouse. It may be that once we cover the general case we don't need anything separate for FrameView. > Hooking up a fake mouse event is expedient, if a little repugnant; decoupling > the code is a bit fragile and goes pretty deep. (E.g., on resize, the mouse may > end up over a scrollbar, so you can't just ignore the scrollbars.) My main complaint about your initial patch was that making a fake mouse event in response to a real wheel event is wrong. The major way it's wrong is (a). It's wrong to say that a wheel event is causing this. It's the *response* to the wheel event that creates the need to update things like cursors and makes you want to send a simulated mouse move event. You're focusing on issue (c), which is whether a fake mouse event is the best way to trigger the new processing. We can be flexible on this, but ideally the code is clear and not full of "fake" anything. I really don't think it's all that difficult to clean this up and make it right. The degree to which updating cursors is tied to a PlatformEvent for a mouse move event is actually pretty small in my opinion.
[Just for clarity, I'm not the originator of the original patch....] (In reply to comment #27) > a) How we detect that there's a change that requires some mouse position > processing. I guess there are two approaches to this (and intermediates between the two extremes): (i) Code which does anything which causes changes which require such processing initiate the processing. Advantages: efficient (no extra checking needed). Disadvantages: need to carefully wire up lots of code. (ii) Check whether such processing is needed very often (possibly prompted by code which does anything which remotely *may* cause such a need), and do the processing if necessary. Advantages: no need to wire up other code so carefully. Disadvantages: nontrivial processing and possibly inefficient. In my quick-and-dirty patch, I've taken route (i): EventHandler::mouseMovedVirtually() is a stub which tells the EventHandler that something happened which caused the mouse to move "virtually". Currently, this immediately fires off a fake mouse event (ugh), but upon decoupling the code it could do something better; it could also be set to start a timer (to coalesce), etc. > b) What should be seen at the DOM level. [...] Right. I (as opposed to the original patch's author) wired up a fake mouse move purely out of expediency -- it's clearly not right. It forces a policy decision (send mouse move events on "virtual" mouse moves), but we shouldn't let a hack make policy. That said, sending a DOM mouse move may be desired, after all. > What do we want seen at the DOM level when the option key goes down? Probably > not a "mouse move" event, so that's a minor bug on the Mac that would be nice > to fix. Unless we decide it's a feature rather than a bug. Seems unlikely that that's a feature! I wouldn't want the option key to be hooked up to virtual mouse moves either, so that processing should probably be separated. > It may be expedient to use what looks like a "mousemove" event to minimize > refactoring of the existing code. But I think that sort of lack of clarity and > logic in the code is unnecessary. We should be willing to make some changes to > make this logical unless it's so difficult that it makes the problem a lot > harder to fix. It's a little painful since we have ugly scrollbar, etc. code which would also need to be rewired, the details which are a bit painful to get right. Currently, a lot of code is wired up to take a PlatformMouseEvent, probably unnecessarily. > It would be very useful to determine where those events come from. Is it the > Mac OS X AppKit that is providing these events? Or is it the Mac-specific > WebKit code that creates them? If the former, then we should just live with > them as is. If it's the latter, maybe we can solve this in a platform > independent way and get rid of the unneeded code. I wouldn't want to bet my life on it, but I would guess the former, or actually code in Safari itself. I don't think I've seen code that does this in WebKit (and Chromium/Mac doesn't get extra events). > > When > > scrolling using the keyboard, in Safari/Mac, the cursor (and tooltip) > > disappear, thereby eliminating a bunch of problems; [...] > > This is an interesting point. It hides some problems (cursor and tooltip) but > doesn't affect others at all (status bar, DOM events as seen on the web page, > hover effects in the page). True, and of course it doesn't help with scrolling with trackpad/mousewheel/other either. > Why don't other platforms have this behavior? Is this really a unique feature > of Mac OS X, or is this something we'd want on Windows and Unix desktop systems > too? It's standard on Mac, doesn't happen on Windows (in any browser that I know of, at least). I don't know about Unix desktops.... > I don't think this refactoring and clean-up is particularly difficult, and I'd > love to see it done for code clarity either before or after this bug is fixed. > I agree that it doesn't have to be done to fix the bug. Agreed, which is why my (even quick-and-dirty) patch is written as it is. For the mouse-related stuff (I don't know about the option key, etc.), I would separate out the code in mouseMoved() and handleMouseMoveEvent(), and have them (and mouseMovedVirtually()) call down to the common code appropriately. > This seems to cover only a tiny fraction of problem (a). It's true that a > FrameView scrolling can move an element. But also, changes to the element's > geometry can move it. And overflow area scrolling can also move it. I'd like to > understand our general strategy to detecting when elements move out from under > the mouse. It may be that once we cover the general case we don't need anything > separate for FrameView. I suppose that, adopting my tactic (i), we'd have to wire up all these other things explicitly as well. Our "general strategy for detecting when elements move out from under the mouse" rests right now entirely in EventHandler (not so great, but everything else seems mostly unaware of the mouse!), and in handleMouseMoveEvent() in particular (at least that should be changed). A further possible issue is that we may need to know whenever the mouse moves relative to anything (leaving an element); this would be the case if we want to send DOM mouse moves when scrolling, for example. > My main complaint about your initial patch was that making a fake mouse event > in response to a real wheel event is wrong. The major way it's wrong is (a). > It's wrong to say that a wheel event is causing this. It's the *response* to > the wheel event that creates the need to update things like cursors and makes > you want to send a simulated mouse move event. [Not my original patch, which is why I wired up to the scrolling instead....] So what do you think of having a separate mouseMovedVirtually() method in EventHandler? As I said, it and mouseMoved()/etc. should be refactored, but the semantics of mouseMovedVirtually() are quite clear -- whenever the mouse moves relative to any elements (without the mouse actually moving -- i.e., the elements move!), it should be called. Currently, I call it from the scrolling code, but it should be called from elsewhere as well; it may need to be made more sophisticated to work correctly in those cases, but it's a start.
Sorry for confusing you with the earlier person working on this bug! (In reply to comment #28) > So what do you think of having a separate mouseMovedVirtually() method in > EventHandler? I would name it simulateMouseMove() or simulateMouseMotion() since I don't really think "virtually" is quite as good a way of saying what the function does. > but the > semantics of mouseMovedVirtually() are quite clear -- whenever the mouse moves > relative to any elements (without the mouse actually moving -- i.e., the > elements move!), it should be called. If that’s true, then perhaps we should name it for when it should be called rather than for what it does. Maybe contentMovedUnderMouse() or something along those lines? > Currently, I call it from the scrolling > code, but it should be called from elsewhere as well; it may need to be made > more sophisticated to work correctly in those cases, but it's a start. Seems like a reasonable first step. Lets not set our sights too low though. Lets try to fix this all the way!
(In reply to comment #29) > Sorry for confusing you with the earlier person working on this bug! No worries. :-) > If that’s true, then perhaps we should name it for when it should be called > rather than for what it does. Maybe contentMovedUnderMouse() or something along > those lines? That sounds like a good name. > Seems like a reasonable first step. Lets not set our sights too low though. > Lets try to fix this all the way! So an issue is that, as I said, the PlatformMouseEvent's go fairly deep. In particular, we need to tell our Scrollbar's that the mouse position has changed relative to them. (This isn't true for the window scrollbars for the scrolling case, but is needed for a resize; for example, if the resize causes the mouse to be over or to leave the scrollbar, it may need to update its hover state.) Now, the stuff in Scrollbar.cpp isn't a problem, but it calls down to ScrollbarTheme ... which involves code in many places. There's also the mouse event dispatch code (going down into the DOM code), though that looks more easily manageable. Possible solutions: (1) Refactor everything, ditching PlatformMouseEvent's where possible. (Seems a bit like overkill? This would affect code in many places, including platform code.) (2) Add methods which don't require PlatformMouseEvent's alongside the current ones. (Adds either hard-to-maintain redundancy -- or complexity, if code is refactored to make things common. Not really any better than (1), since it would also touch platform code.) (3) Add an event type to PlatformMouseEvent -- "MouseEventUpdatePosition" or some such; code which cares can look for it. (Possible bugs in code which assumes all data are valid, but that would happen with fake mouse moves anyway.) (4) Generate fake PlatformMouseEvent's (with a fake MouseEventMoved). Obviously "works", but it's a pack of lies. I'm kind of inclined towards (3). For things like keyboard flags, one could similarly have "MouseEventUpdateKeys".
*** Bug 26727 has been marked as a duplicate of this bug. ***