Bug 114446 - Unset :hover in inner documents
Summary: Unset :hover in inner documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on: 114912
Blocks: 98168
  Show dependency treegraph
 
Reported: 2013-04-11 08:27 PDT by Allan Sandfeld Jensen
Modified: 2013-04-21 04:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.85 KB, patch)
2013-04-11 08:31 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 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.
Comment 1 Allan Sandfeld Jensen 2013-04-11 08:31:14 PDT
Created attachment 197592 [details]
Patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Allan Sandfeld Jensen 2013-04-12 07:16:18 PDT
Created attachment 197847 [details]
Patch
Comment 9 kov's GTK+ EWS bot 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
Comment 10 Allan Sandfeld Jensen 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?
Comment 11 Antonio Gomes 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?
Comment 12 Allan Sandfeld Jensen 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.
Comment 13 Antonio Gomes 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
Comment 14 Allan Sandfeld Jensen 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.
Comment 15 Allan Sandfeld Jensen 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>
Comment 16 Allan Sandfeld Jensen 2013-04-18 06:36:15 PDT
All reviewed patches have been landed.  Closing bug.