Hovered node gets stuck on touch events

• Component: UI Events
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 125105 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Allan Sandfeld Jensen]

Created attachment 125111 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 125117 [details]
Patch

One line update to chromium port, so it compiles.

[Antonio Gomes]

Comment on attachment 125117 [details]
Patch

we do it much simplier. I will paste a patch...

[WebKit Review Bot]

Comment on attachment 125117 [details]
Patch

Clearing flags on attachment: 125117

Committed r106554: <http://trac.webkit.org/changeset/106554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

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.

[Allan Sandfeld Jensen]

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

[Nate Chapin]

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

[Allan Sandfeld Jensen]

Created attachment 125295 [details]
Patch

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Allan Sandfeld Jensen]

Created attachment 125300 [details]
Fixup

[WebKit Review Bot]

Comment on attachment 125300 [details]
Fixup

Clearing flags on attachment: 125300

Committed r106649: <http://trac.webkit.org/changeset/106649>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

Comment on attachment 125117 [details]
Patch

patch changes a lot more than needed to achieve this.

it also does unrelated renames... :/

[James Robinson]

I don't think this is the behavior we want to have for touch events in chromium.  Touch interactions should not trigger :active behavior.

[Allan Sandfeld Jensen]

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