NEW 131172
Mouse Cursor doesn't change appropriately.
https://bugs.webkit.org/show_bug.cgi?id=131172
Summary Mouse Cursor doesn't change appropriately.
Nathan Hammond
Reported 2014-04-03 10:56:38 PDT
Visit: http://emberjs.jsbin.com/nosap/1 Go to the bottom of the page, find the "NODES" link. Hover over it, move cursor off to the right of the element. Cursor will now remain as "pointer" until it intersects with a different element. This likely has something to do with the applied transform. In order to see this bug you MUST NOT have flexbox enabled without prefixes.
Attachments
Patch (1.57 KB, patch)
2014-04-07 05:36 PDT, Miyoung Shin
no flags
Patch (4.18 KB, patch)
2014-04-11 05:02 PDT, Miyoung Shin
no flags
Patch (4.11 KB, patch)
2014-04-12 18:07 PDT, Miyoung Shin
no flags
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (462.90 KB, application/zip)
2014-04-12 19:08 PDT, Build Bot
no flags
Patch (4.18 KB, patch)
2014-04-13 00:41 PDT, Miyoung Shin
no flags
Patch (7.25 KB, patch)
2014-07-15 08:02 PDT, Miyoung Shin
no flags
Patch (4.89 KB, patch)
2014-07-15 08:37 PDT, Miyoung Shin
no flags
Miyoung Shin
Comment 1 2014-04-06 02:57:36 PDT
Is there anyone to follow this issue ? If not, can I pick this one ?
Miyoung Shin
Comment 2 2014-04-07 05:36:08 PDT
zalan
Comment 3 2014-04-08 07:42:55 PDT
Please add a test case.
Miyoung Shin
Comment 4 2014-04-08 08:08:13 PDT
do you mean new layout test case for this issue?
Miyoung Shin
Comment 5 2014-04-09 08:42:32 PDT
I will make new layout test case. please let me know if you don't mean that.
zalan
Comment 6 2014-04-09 08:43:51 PDT
(In reply to comment #5) > I will make new layout test case. Thanks.
Miyoung Shin
Comment 7 2014-04-11 05:02:32 PDT
Darin Adler
Comment 8 2014-04-12 12:43:03 PDT
Comment on attachment 229125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229125&action=review > Source/WebCore/page/EventHandler.cpp:1325 > + // Mouse pointer is possible to stay within the page even if hitTest's result is empty. > + // Cursor should be reset by the pointer type I don’t understand this comment. Why is there no code change to go with it?
Miyoung Shin
Comment 9 2014-04-12 18:03:51 PDT
(In reply to comment #8) > I don’t understand this comment. Why is there no code change to go with it? Sorry, I've changed wrong code. I will reupload it soon.
Miyoung Shin
Comment 10 2014-04-12 18:07:49 PDT
Build Bot
Comment 11 2014-04-12 19:08:41 PDT
Comment on attachment 229214 [details] Patch Attachment 229214 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4560572200779776 New failing tests: fast/events/mouse-cursor-reset.html
Build Bot
Comment 12 2014-04-12 19:08:44 PDT
Created attachment 229216 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Miyoung Shin
Comment 13 2014-04-13 00:41:31 PDT
Miyoung Shin
Comment 14 2014-05-25 18:13:01 PDT
darin, could you take a look?
Miyoung Shin
Comment 15 2014-07-15 08:02:19 PDT
Miyoung Shin
Comment 16 2014-07-15 08:37:01 PDT
Darin Adler
Comment 17 2014-07-20 08:57:43 PDT
Comment on attachment 234930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234930&action=review Can’t review because the patch doesn’t explain itself sufficiently in comments or in change log. > Source/WebCore/rendering/RenderLayer.cpp:4547 > + if (!request.isChildFrameHitTest() && (request.active() || request.release() || (request.move() && renderer().frame().isMainFrame())) && isRootLayer()) { I don’t understand how this change relates to the change log comment. This says that the main frame is a special case, and that the “move” flag is relevant. Nothing in the change log helps explain why that’s correct to consider. My guess is that for subframes we’d like to fall back to the main frame and that’s why the isMainFrame test is relevant, but I’m not sure that’s the reason. And I have no idea why checking the move() boolean is relevant.
Miyoung Shin
Comment 18 2014-07-21 01:34:02 PDT
I'm sorry to remain the insufficient description in the change log comment. Let me give you a explain in more details for this patch. This is a test page's layout. | | | ... | | HTML & Body's Rendering box | |__________________________________________| |button| x |______| ↑ |___ there isn't any element here. Currently, we can see the 'null' value when getting the result of hitTest for MouseMove event on x coordinates, and can't update mouse cursor like other browsers if mouse cursor moves the button coordinates to the x coordinates. I believe that the visible expectation is to change a hand pointer of mouse cursor to a general pointer. But I met with a difficulty like below. - Should we cope with "null" value ? - or, Do we guarantee that hitTest shouldn't return "null" value? According your opinion, I tried to dig out second question. I refereed the document.elementFromPoint's behaviour because we can see HTML Element value as a result of elementFromPoint under the same conditions. I think you've already known as far as here. I found that in case of elementFromPoint hitTestRequest is set by "Active" option, and the hitTest result updates to HTML Element again as the hitTest's result is "null". So, I added "Move()" flag at there to ensure HTML Element as hitTest result for MouseMove event like elementFromPoint's active() option. In case of isMainFrame, your guess is right. I intended to do so because this change should have effect on only main frame. If I remove isMainFrame condition, it will have a bad effect to mouseEnter & MouseLeave event's result. Anyway, I don't think we need this change for sub frame. Thank you for your feedback and please let me know if I'm wrong.
Darin Adler
Comment 19 2014-07-21 17:12:00 PDT
(In reply to comment #18) > I found that in case of elementFromPoint hitTestRequest is set by "Active" option, and the hitTest result updates to HTML Element again as the hitTest's result is "null". > > So, I added "Move()" flag at there to ensure HTML Element as hitTest result for MouseMove event like elementFromPoint's active() option. OK. This doesn’t just affect cursor adjustment. It affects mousemove DOM events too. So is that OK? Does that match other browsers’ behavior? Can we make a test case?
Miyoung Shin
Comment 20 2014-07-22 20:57:56 PDT
(In reply to comment #19) > OK. This doesn’t just affect cursor adjustment. It affects mousemove DOM events too. So is that OK? Does that match other browsers’ behavior? Can we make a test case? At first, I focused to check the mouse cursor's type and as I mentioned before, we works different with others. When mouse cursor moves button coordinates and x coordinates, FF & chrome & IE10 : it changes the cursor type from hand type to arrow type. Safari : it doesn't change the cursor type without my patch. But I realized that all browsers have a different layout result from document.elementFromPoint() when I made a test case for mousemove event. - FF : for mainframe and subframe when mouse cursor is on x coordinates, HTML element is hit. - IE10 : under the same condition, Body element is hit. - Chrome : nothing. It seems that they are handling just the null value of hitTestResult. - Safari : nothing, too. with this patch, for main frame, HTML element is hit. Do I need to follow FF way? If then, I need to change my patch to remove inMainFrame condition to work like FF unlike originally intention. Of cause, I guess I need to check mouseenter/leave event as well because it will be affected by the hitTest's Result. I'd like to seek your advice. Thanks.
Michael Catanzaro
Comment 21 2016-09-17 07:11:11 PDT
Comment on attachment 234930 [details] Patch Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.
Note You need to log in before you can comment on or make changes to this bug.