Bug 114446

Summary: Unset :hover in inner documents
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: UI EventsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, gtk-ews, ojan.autocc, rniwa, tonikitoo, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 114912    
Bug Blocks: 98168    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch none

Allan Sandfeld Jensen
Reported 2013-04-11 08:27:23 PDT
Commit r145126 introduced a regression in updates of hover/active state in EventHandler::hitTestResultAtPoint. If the old hover node was in an inner document (such as an iframe), and the new hover node is in an different document, the old hover node does not get unset. This is however not a normal use case as mouse-events does not use EventHandler::hitTestResultAtPoint, only touch-events and gesture-events does.
Attachments
Patch (10.85 KB, patch)
2013-04-11 08:31 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (720.45 KB, application/zip)
2013-04-11 13:45 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (689.13 KB, application/zip)
2013-04-11 14:56 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (570.63 KB, application/zip)
2013-04-11 22:25 PDT, Build Bot 		no flags
Details
Patch (10.92 KB, patch)
2013-04-12 07:16 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1 2013-04-11 08:31:14 PDT
Created attachment 197592 [details] Patch
Build Bot
Comment 2 2013-04-11 13:45:38 PDT
Comment on attachment 197592 [details] Patch Attachment 197592 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/45349 New failing tests: http/tests/misc/bubble-drag-events.html editing/pasteboard/drag-and-drop-objectimage-contenteditable.html http/tests/misc/drag-over-iframe-invalid-source-crash.html
Build Bot
Comment 3 2013-04-11 13:45:39 PDT
Created attachment 197663 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Build Bot
Comment 4 2013-04-11 14:56:40 PDT
Comment on attachment 197592 [details] Patch Attachment 197592 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/57058 New failing tests: http/tests/misc/bubble-drag-events.html editing/pasteboard/drag-and-drop-objectimage-contenteditable.html http/tests/misc/drag-over-iframe-invalid-source-crash.html
Build Bot
Comment 5 2013-04-11 14:56:42 PDT
Created attachment 197678 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Build Bot
Comment 6 2013-04-11 22:25:48 PDT
Comment on attachment 197592 [details] Patch Attachment 197592 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/125005 New failing tests: http/tests/misc/embed-image-load-outlives-gc-without-crashing.html
Build Bot
Comment 7 2013-04-11 22:25:50 PDT
Created attachment 197715 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Allan Sandfeld Jensen
Comment 8 2013-04-12 07:16:18 PDT
Created attachment 197847 [details] Patch
kov's GTK+ EWS bot
Comment 9 2013-04-12 08:17:15 PDT
Comment on attachment 197847 [details] Patch Attachment 197847 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/46154
Allan Sandfeld Jensen
Comment 10 2013-04-12 10:59:18 PDT
(In reply to comment #9) > (From update of attachment 197847 [details]) > Attachment 197847 [details] did not pass gtk-ews (gtk): > Output: http://webkit-queues.appspot.com/results/46154 It looks like the build broke in a dependency. Nothing to do with this patch. Or I am missing something?
Antonio Gomes
Comment 11 2013-04-18 06:21:28 PDT
Comment on attachment 197847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197847&action=review > Source/WebCore/page/EventHandler.cpp:3990 > + if (!shouldGesturesTriggerActive() && allTouchReleased) reading the function name I had the impression that this if block should be testing if (shouldGestureTriggerActive and allTouchReleased) instead like, it had a 'active' before and now that all touch were released, it is cleaning :active/:hover up. Did I read it wrong?
Allan Sandfeld Jensen
Comment 12 2013-04-18 06:26:28 PDT
(In reply to comment #11) > (From update of attachment 197847 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197847&action=review > > > Source/WebCore/page/EventHandler.cpp:3990 > > + if (!shouldGesturesTriggerActive() && allTouchReleased) > > reading the function name I had the impression that this if block should be testing if (shouldGestureTriggerActive and allTouchReleased) instead > > like, it had a 'active' before and now that all touch were released, it is cleaning :active/:hover up. > > Did I read it wrong? shouldGesturesTriggerActive() indicates that :active is set with a maybe-tap gesture event. When it returns false it means touch-events sets active (because gesture events are not enabled or maybe-tap not yet implemented). Since this is in touch handling, we shouldn't update :active if it returns true.
Antonio Gomes
Comment 13 2013-04-18 06:32:20 PDT
(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 197847 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=197847&action=review > > > > > Source/WebCore/page/EventHandler.cpp:3990 > > > + if (!shouldGesturesTriggerActive() && allTouchReleased) > > > > reading the function name I had the impression that this if block should be testing if (shouldGestureTriggerActive and allTouchReleased) instead > > > > like, it had a 'active' before and now that all touch were released, it is cleaning :active/:hover up. > > > > Did I read it wrong? > > shouldGesturesTriggerActive() indicates that :active is set with a maybe-tap gesture event. When it returns false it means touch-events sets active (because gesture events are not enabled or maybe-tap not yet implemented). Since this is in touch handling, we shouldn't update :active if it returns true. Gestures x Touch. Now that Google is gone, it might be room to clean up "gestures" since it is their baby
Allan Sandfeld Jensen
Comment 14 2013-04-18 06:34:15 PDT
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 197847 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=197847&action=review > > > > > > > Source/WebCore/page/EventHandler.cpp:3990 > > > > + if (!shouldGesturesTriggerActive() && allTouchReleased) > > > > > > reading the function name I had the impression that this if block should be testing if (shouldGestureTriggerActive and allTouchReleased) instead > > > > > > like, it had a 'active' before and now that all touch were released, it is cleaning :active/:hover up. > > > > > > Did I read it wrong? > > > > shouldGesturesTriggerActive() indicates that :active is set with a maybe-tap gesture event. When it returns false it means touch-events sets active (because gesture events are not enabled or maybe-tap not yet implemented). Since this is in touch handling, we shouldn't update :active if it returns true. > > Gestures x Touch. Now that Google is gone, it might be room to clean up "gestures" since it is their baby We also use gestures in the Qt port, and while we are not yet using GestureTapDown, I do believe it should be implemented at some point.
Allan Sandfeld Jensen
Comment 15 2013-04-18 06:36:10 PDT
Comment on attachment 197847 [details] Patch Clearing flags on attachment: 197847 Committed r148672: <http://trac.webkit.org/changeset/148672>
Allan Sandfeld Jensen
Comment 16 2013-04-18 06:36:15 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.