Unset :hover in inner documents

• Component: UI Events
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 197592 [details]
Patch

[Build Bot]

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]

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 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]

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 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]

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]

Created attachment 197847 [details]
Patch

[kov's GTK+ EWS bot]

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]

(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 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]

(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]

(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]

(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 on attachment 197847 [details]
Patch

Clearing flags on attachment: 197847

Committed r148672: <http://trac.webkit.org/changeset/148672>

[Allan Sandfeld Jensen]

All reviewed patches have been landed.  Closing bug.