Bug 77620 - Hovered node gets stuck on touch events
Summary: Hovered node gets stuck on touch events
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:
Blocks:
 
Reported: 2012-02-02 02:20 PST by Allan Sandfeld Jensen
Modified: 2013-02-05 10:32 PST (History)
7 users (show)

See Also:


Attachments
Patch (12.54 KB, patch)
2012-02-02 04:08 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2012-02-02 04:30 PST, Allan Sandfeld Jensen
kenneth: review+
Details | Formatted Diff | Diff
Patch (13.88 KB, patch)
2012-02-02 05:01 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (2.28 KB, patch)
2012-02-03 02:23 PST, Allan Sandfeld Jensen
kenneth: review+
Details | Formatted Diff | Diff
Fixup (2.48 KB, patch)
2012-02-03 02:58 PST, 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 2012-02-02 02:20:33 PST
If you touch click on a node and the move the finger off the node (make sure the page is not scrollable, otherwise the finger will pan), the node will continue to have hovered state after you lift the finger.

This can be demonstrated on any page with links.
Comment 1 Allan Sandfeld Jensen 2012-02-02 04:08:58 PST
Created attachment 125105 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-02-02 04:20:09 PST
Comment on attachment 125105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125105&action=review

> Source/WebCore/rendering/HitTestRequest.h:55
> +    // Convinience functions

// Conv_e_nience functions_._

> Source/WebCore/rendering/RenderLayer.cpp:4050
> +    // Check to see if the hovered node has changed. If not, then we don't need to
> +    // do anything.

I would merge that to one line. It fits the length of the other comments
Comment 3 Allan Sandfeld Jensen 2012-02-02 04:30:04 PST
Created attachment 125111 [details]
Patch
Comment 4 Allan Sandfeld Jensen 2012-02-02 05:01:58 PST
Created attachment 125117 [details]
Patch

One line update to chromium port, so it compiles.
Comment 5 Antonio Gomes 2012-02-02 06:06:24 PST
Comment on attachment 125117 [details]
Patch

we do it much simplier. I will paste a patch...
Comment 6 WebKit Review Bot 2012-02-02 07:10:04 PST
Comment on attachment 125117 [details]
Patch

Clearing flags on attachment: 125117

Committed r106554: <http://trac.webkit.org/changeset/106554>
Comment 7 WebKit Review Bot 2012-02-02 07:10:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Csaba Osztrogonác 2012-02-02 09:53:12 PST
Reopen, because it made 4 tests crash in debug mode:
- fast/events/touch/basic-multi-touch-events.html
- fast/events/touch/touch-target.html
- fast/events/touch/touch-before-pressing-spin-button.html
- fast/events/touch/touch-coords-in-zoom-and-scroll.html

SHOULD NEVER BE REACHED
../../../../Source/WebCore/page/EventHandler.cpp(3253) : bool WebCore::EventHandler::handleTouchEvent(const WebCore::PlatformTouchEvent&)

Could you check these crashes and fix?
Comment 9 Csaba Osztrogonác 2012-02-02 09:55:31 PST
Comment on attachment 125117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125117&action=review

> Source/WebCore/page/EventHandler.cpp:3253
> +            ASSERT_NOT_REACHED();

It seems this assertion hit. But why?
Comment 10 Csaba Osztrogonác 2012-02-02 10:04:52 PST
I skipped them to paint the debug bot green: http://trac.webkit.org/changeset/106567/trunk/LayoutTests/platform/qt/Skipped

Please unskip them with the proper fix.
Comment 11 Allan Sandfeld Jensen 2012-02-02 11:55:01 PST
(In reply to comment #9)
> (From update of attachment 125117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125117&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:3253
> > +            ASSERT_NOT_REACHED();
> 
> It seems this assertion hit. But why?

There is a fifth touchpoint state; stationary. We don't really send them here, but I guess they can be inserted by javascript, and it should be handled anyway in case other platforms send it to handleTouchEvent.

Anyway the fix is easy: Just remove the assertion, or add a check for the last touchpoint state. The assertion is not critical, just a common practice in the bottom of a switch.
Comment 12 Nate Chapin 2012-02-02 12:36:14 PST
(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 125117 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=125117&action=review
> > 
> > > Source/WebCore/page/EventHandler.cpp:3253
> > > +            ASSERT_NOT_REACHED();
> > 
> > It seems this assertion hit. But why?
> 
> There is a fifth touchpoint state; stationary. We don't really send them here, but I guess they can be inserted by javascript, and it should be handled anyway in case other platforms send it to handleTouchEvent.
> 
> Anyway the fix is easy: Just remove the assertion, or add a check for the last touchpoint state. The assertion is not critical, just a common practice in the bottom of a switch.

Chromium is hitting this assertion too.  See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=touch-target.html%20touch-coords-in-zoom-and-scroll.html%20basic-multi-touch-events.html

Will mark as expected failures in the meantime.
Comment 13 Allan Sandfeld Jensen 2012-02-03 02:23:37 PST
Created attachment 125295 [details]
Patch
Comment 14 WebKit Review Bot 2012-02-03 02:26:40 PST
Attachment 125295 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Kenneth Rohde Christiansen 2012-02-03 02:29:37 PST
Comment on attachment 125295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125295&action=review

> Source/WebCore/ChangeLog:10
> +
> +        * page/EventHandler.cpp:
> +        (WebCore::eventNameForTouchPointState): Explicitly show how TouchStationary is handled.
> +        (WebCore::EventHandler::handleTouchEvent): Remove TouchStationary from ASSERT.
> +

Maybe you could write that you are adding code so that it is like before you last patch?

> Source/WebCore/page/EventHandler.cpp:3200
> +    case PlatformTouchPoint::TouchStationary:
> +        // TouchStationary state is not converted to touch events.
>      default:
>          ASSERT_NOT_REACHED();

It could be a bit more obvious that the ASSERT is also meant for the TouchStationary
Comment 16 Allan Sandfeld Jensen 2012-02-03 02:58:05 PST
Created attachment 125300 [details]
Fixup
Comment 17 WebKit Review Bot 2012-02-03 04:49:32 PST
Comment on attachment 125300 [details]
Fixup

Clearing flags on attachment: 125300

Committed r106649: <http://trac.webkit.org/changeset/106649>
Comment 18 WebKit Review Bot 2012-02-03 04:49:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Antonio Gomes 2012-02-03 13:45:57 PST
Comment on attachment 125117 [details]
Patch

patch changes a lot more than needed to achieve this.

it also does unrelated renames... :/
Comment 20 James Robinson 2013-02-05 09:59:43 PST
I don't think this is the behavior we want to have for touch events in chromium.  Touch interactions should not trigger :active behavior.
Comment 21 Allan Sandfeld Jensen 2013-02-05 10:32:57 PST
(In reply to comment #20)
> I don't think this is the behavior we want to have for touch events in chromium.  Touch interactions should not trigger :active behavior.

Then set the hit-test performed to ReadOnly. Btw, this bug was only about clearing the state when releasing the touch. I don't know where it was originally introduced to be set on touch down.