Bug 96060 – Trigger node active state with touch gesture events

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Rick Byers]

Today elements can be marked active (i.e. :active CSS pseudo-class) by mouse events and by touch events.  However, not all ports send touch events by default (eg. chrome sends them only when there are touch listeners on the document, since supporting low-level touch events can significantly hurt scroll performance).  For ports that support GESTURE_EVENTS, we should let the appropriate gesture events trigger the active state instead.

In particular, GestureTapDown should trigger active, and we should add a new GestureTapCancel (in addition to GestureTap) that removes the active state.

[Rick Byers]

Created attachment 162974 [details]
Patch

[Rick Byers]

Created attachment 162975 [details]
Combined with 96183 for EWS

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Rick Byers]

aelias: I still need to add LayoutTests for this, but they should be pretty straight forward - I'll get them in early next week.  No non-chromium port appears to use GestureTapDown, so I believe it's only desktop and android chrome we need to worry about here.

I hope to have the desktop (Windows/ChromeOS) side landed early next week (it's mostly up for review here: http://codereview.chromium.org/10928066/, one more small piece once bug 96183 lands).  Do you want me to block on Android support for GestureTapCancel?  Without it, I believe nodes would get stuck in the 'active' state when you send tapDown but not tap (but they'd be cleared as soon as there was another tapDown or mousemove/mousedown event).  Definitely not what you want, but not a disaster.  As we've discussed, we'd really like this in Windows/ChromeOS for M23 (a week from now).

[WebKit Review Bot]

Comment on attachment 162975 [details]
Combined with 96183 for EWS

Attachment 162975 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13788840

New failing tests:
fast/events/touch/touch-active-state.html

[James Robinson]

Does this also mean that touch events won't change active state, at least on ports that send GestureTap*?  If not, how do we deal with the touch and gesture events on pages that do listen to some touch events?

[Rick Byers]

(In reply to comment #6)
> Does this also mean that touch events won't change active state, at least on ports that send GestureTap*?  If not, how do we deal with the touch and gesture events on pages that do listen to some touch events?

Yes, that was my intention - it doesn't make sense to do it on both touch and gesture events (eg. pinch would be even more bizarre).  However, I don't have any good way to tell if a port sends GestureTapDown (AFAIK), so at the  moment this is implemented to turn of touch-based active states on any port that uses ENABLE(GESTURE_EVENTS).  This is probably wrong for the Qt port (which I believe uses touch and GESTURE_EVENTS, but not TapDown/TapCancel).  I was thinking I need to add a new ENABLE condition here ("GESTURE_EVENTS_TRIGGER_ACTIVE" or something), what do you think?

[Rick Byers]

Created attachment 163438 [details]
Add new layout test and updated expectations

[WebKit Review Bot]

Comment on attachment 163438 [details]
Add new layout test and updated expectations

Attachment 163438 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13806932

[Rick Byers]

Created attachment 163476 [details]
Fixes, and opt-in using platform ifdefs per jamesr

[Rick Byers]

Created attachment 163498 [details]
Add iframe-based layout test

[Rick Byers]

Comment on attachment 163498 [details]
Add iframe-based layout test

Ping - who is the right person to review this change?  ABarth perhaps?

[Rick Byers]

CC allan.jensen since he's actively changing related code (updateHoverActiveState, hit testing)

[Adam Barth]

Comment on attachment 163498 [details]
Add iframe-based layout test

I looked at this patch, but I don't feel like I understand this topic well enough to review it.

[James Robinson]

I could make at least an honest effort at this, but it'll probably be early next week - I'm gardening today and tomorrow.  Perhaps you could take a look through the svn logs and see who else has reviewed code in this area lately?

[Allan Sandfeld Jensen]

If you want to go in this direction, I would like propose something more bold. Basically create a new pseudo-class maybe called -webkit-touched, and set that on tap-down and use it to replace link highlighting.

[Rick Byers]

(In reply to comment #15)
> I could make at least an honest effort at this, but it'll probably be early next week - I'm gardening today and tomorrow.  Perhaps you could take a look through the svn logs and see who else has reviewed code in this area lately?

Thanks guys.  Antonio, it looks like you've reviewed related patches lately, can you take a look please?

[Rick Byers]

(In reply to comment #16)
> If you want to go in this direction, I would like propose something more bold. Basically create a new pseudo-class maybe called -webkit-touched, and set that on tap-down and use it to replace link highlighting.

Thanks Allen.  I agree, the idea of implementing link highlighting with this is very appealing.  The link highlighting work we've (wjmaclean) been doing for chromium shares the gestures (TapDown/TapCancel), but perhaps there is an opportunity to share more - but there are some non-trivial constraints (eg. to ensure GPU acceleration on Android).

Regardless of any new pseudo-class, we'd still want to set :active via touch though (eg. lots of sites have buttons that look depressed on :active).  Note that :active is already set on touch events, but has some problems (see my ChangeLog), so this just provides an alternate / improved mechanism for the existing functionality.

Any objections to this change as a first step here?  I'm anxious to get this working (provided the approach is sound of course).

[Allan Sandfeld Jensen]

Comment on attachment 163498 [details]
Add iframe-based layout test

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

The patch looks solid enough. I only have a few comments.

> Source/WebCore/dom/GestureEvent.cpp:61
> +    case PlatformEvent::GestureTapCancel:

You can not cancel a Tap, so perhaps TapDownCancel, or CancelTapDown would be a more accurate name.

> Source/WebCore/page/EventHandler.cpp:2402
> +        IntPoint hitTestPoint = m_frame->view()->windowToContents(gestureEvent.position());

This looks new, why is this change necessary?

[Allan Sandfeld Jensen]

(In reply to comment #18)
> Any objections to this change as a first step here?  I'm anxious to get this working (provided the approach is sound of course).

No, I just wanted to put the idea in your head while you were thinking about this logic already :)

[Antonio Gomes]

Comment on attachment 163498 [details]
Add iframe-based layout test

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

Looks good. Some questions ...

>> Source/WebCore/dom/GestureEvent.cpp:61
>> +    case PlatformEvent::GestureTapCancel:
> 
> You can not cancel a Tap, so perhaps TapDownCancel, or CancelTapDown would be a more accurate name.

Agreed: "Tap" is the action of finger down + finger up with a minimum radio movement.

> Source/WebCore/page/EventHandler.cpp:2367
> +bool EventHandler::shouldGesturesTriggerActive()

would shouldTouchEventsTriggerActive be more accurate?

>> Source/WebCore/page/EventHandler.cpp:2402
>> +        IntPoint hitTestPoint = m_frame->view()->windowToContents(gestureEvent.position());
> 
> This looks new, why is this change necessary?

Please answer.

[Rick Byers]

(In reply to comment #19)
> (From update of attachment 163498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163498&action=review
> 
> The patch looks solid enough. I only have a few comments.
> 
> > Source/WebCore/dom/GestureEvent.cpp:61
> > +    case PlatformEvent::GestureTapCancel:
> 
> You can not cancel a Tap, so perhaps TapDownCancel, or CancelTapDown would be a more accurate name.

Sure.  I'll change PlatformGestureEvent here, but do chromium's WebGestureEvent separately since it's a breaking change to chromium that'll require a 3-step change.
 
> > Source/WebCore/page/EventHandler.cpp:2402
> > +        IntPoint hitTestPoint = m_frame->view()->windowToContents(gestureEvent.position());
> 
> This looks new, why is this change necessary?

There was a pre-existing bug here that had gone unnoticed - when the page is scrolled gestures get targeted at the wrong node.  I considered splitting this out into it's own bug/fix, but I thought the code wasn't being used for much else in practice (we hadn't seen an issue).  I cover this case in my iframe layout test (although the issues isn't specific just to iframes).

Digging into it deeper now, I think I should be able to provoke the bug with scrolling on the software path (in chromium we mostly try to do scrolling via our GPU compositor).  I'll work on creating an independent repro for it.  Should I block this CL on that, or is it OK to submit the fix here?

[Rick Byers]

(In reply to comment #21)
> (From update of attachment 163498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163498&action=review
> 
> Looks good. Some questions ...
> 
> >> Source/WebCore/dom/GestureEvent.cpp:61
> >> +    case PlatformEvent::GestureTapCancel:
> > 
> > You can not cancel a Tap, so perhaps TapDownCancel, or CancelTapDown would be a more accurate name.
> 
> Agreed: "Tap" is the action of finger down + finger up with a minimum radio movement.
> 
> > Source/WebCore/page/EventHandler.cpp:2367
> > +bool EventHandler::shouldGesturesTriggerActive()
> 
> would shouldTouchEventsTriggerActive be more accurate?

No, here we're deciding to use gesture events (TapDown,TapDownCancel,Tap) to trigger the active stead INSTEAD of touch events (touchstart/touchend).  I could invert the semantics though and then that name would be OK (I prefer to think of this as opting into gesture-based activation though).

> >> Source/WebCore/page/EventHandler.cpp:2402
> >> +        IntPoint hitTestPoint = m_frame->view()->windowToContents(gestureEvent.position());
> > 
> > This looks new, why is this change necessary?
> 
> Please answer.

[Rick Byers]

(In reply to comment #22)
> > 
> > This looks new, why is this change necessary?
> 
> There was a pre-existing bug here that had gone unnoticed - when the page is scrolled gestures get targeted at the wrong node.  I considered splitting this out into it's own bug/fix, but I thought the code wasn't being used for much else in practice (we hadn't seen an issue).  I cover this case in my iframe layout test (although the issues isn't specific just to iframes).
> 
> Digging into it deeper now, I think I should be able to provoke the bug with scrolling on the software path (in chromium we mostly try to do scrolling via our GPU compositor).  I'll work on creating an independent repro for it.  Should I block this CL on that, or is it OK to submit the fix here?

Sending gesture events to DOM nodes was added (in bug 92281) just for chromium plugins, and it looks like that's still the only place it's exposed.  So I can't repro any issue here with scrolling.

Still, I've forked that issue and fix, and plugin-specific test out into bug 96788 (hopefully I can still get both landed today).

[Rick Byers]

Created attachment 164162 [details]
Patch

[Rick Byers]

Updated patch that includes the rename and doesn't include the fix for bug 96788.  As a result gesture-tap-active-state-iframe fails here.  As soon as bug 96788 lands (it's in the CQ now), I'll merge with it and the test will pass again.

Any other suggestions?

[Rick Byers]

Created attachment 164183 [details]
Merge with trunk

[Rick Byers]

Comment on attachment 164183 [details]
Merge with trunk

This has been merged with the fix for 96788.

[Rick Byers]

Ping - Antonio, any other concerns/questions?

[Antonio Gomes]

Comment on attachment 164183 [details]
Merge with trunk

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

It looks good to me. Before r+ I would have a question:

> Source/WebCore/page/EventHandler.cpp:2377
> +#if PLATFORM(CHROMIUM) && !OS(ANDROID)

I thought you actually wanted this for Androis: the tap down gesture to "activate"  an element. while tapdowncancel cleared the active state.

> Source/WebCore/page/EventHandler.cpp:2401
> +    else
> +        hitType |= HitTestRequest::Move | HitTestRequest::ReadOnly;

the "::Move" here is an unrelated behavior change?

> Source/WebCore/page/EventHandler.h:362
> +    static bool shouldGesturesTriggerActive();

can this be a local static helper instead of a public static class method? Or you thinking of using it from outside?

[Rick Byers]

Comment on attachment 164183 [details]
Merge with trunk

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

>> Source/WebCore/page/EventHandler.cpp:2377
>> +#if PLATFORM(CHROMIUM) && !OS(ANDROID)
> 
> I thought you actually wanted this for Androis: the tap down gesture to "activate"  an element. while tapdowncancel cleared the active state.

Nope, this is Chrome Desktop (Win8 and ChromeOS) only at the moment.  Android wants it too, but their gesture recognizer hasn't been updated to send a GestureTapDownCancel yet, so that'll come later once they update their GR.  They say it should be easy, so I expect this code will soon be simply '#if PLATFORM(CHROMIUM)'.

>> Source/WebCore/page/EventHandler.cpp:2401
>> +        hitType |= HitTestRequest::Move | HitTestRequest::ReadOnly;
> 
> the "::Move" here is an unrelated behavior change?

From my analysis of the code, it seemed that Move|ReadOnly would have the same behavior here as Active|ReadOnly, did I miss something?  Regardless, saying 'Active' for all gestures seemed wrong (eg. pinch updates) - seemed more correct to me to opt into to using Active specifically on TapDown and nowhere else (it's really the only gesture, combined with Tap, that is explicitly about activating an element).

>> Source/WebCore/page/EventHandler.h:362
>> +    static bool shouldGesturesTriggerActive();
> 
> can this be a local static helper instead of a public static class method? Or you thinking of using it from outside?

Yes, a local helper would be sufficient - I'll do that now.

[Allan Sandfeld Jensen]

(In reply to comment #31)
> From my analysis of the code, it seemed that Move|ReadOnly would have the same behavior here as Active|ReadOnly, did I miss something?  Regardless, saying 'Active' for all gestures seemed wrong (eg. pinch updates) - seemed more correct to me to opt into to using Active specifically on TapDown and nowhere else (it's really the only gesture, combined with Tap, that is explicitly about activating an element).
> 
They are all the same when ReadOnly is set, but usually we use Active|ReadOnly as the default.

[WebKit Review Bot]

Comment on attachment 164183 [details]
Merge with trunk

Rejecting attachment 164183 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13878355

[Rick Byers]

Created attachment 164408 [details]
Tweaks based on CR feedback

[Rick Byers]

(In reply to comment #32)
> (In reply to comment #31)
> > From my analysis of the code, it seemed that Move|ReadOnly would have the same behavior here as Active|ReadOnly, did I miss something?  Regardless, saying 'Active' for all gestures seemed wrong (eg. pinch updates) - seemed more correct to me to opt into to using Active specifically on TapDown and nowhere else (it's really the only gesture, combined with Tap, that is explicitly about activating an element).
> > 
> They are all the same when ReadOnly is set, but usually we use Active|ReadOnly as the default.

Ah, glad my understanding of the code was correct.  If that's the convention then I should use it here too, thanks.

[WebKit Review Bot]

Comment on attachment 164408 [details]
Tweaks based on CR feedback

Rejecting attachment 164408 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13863877

[Rick Byers]

(In reply to comment #36)
> (From update of attachment 164408 [details])
> Rejecting attachment 164408 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/13863877

abarth@ can you please review the tiny change in chromium/ here?  This is just plumbing the TapCancel event through to WebCore (you previously approved the CL that added it to WebKit/chromium).

[James Robinson]

The commit queue is complaining because your ChangeLog entry in Source/WebKit/chromium/ChangeLog is malformed, not because you're lacking a reviewer.  Add back the "Reviewed by NOBODY (OOPS!)." line

[Rick Byers]

Created attachment 164432 [details]
Fix ChangeLog

[Rick Byers]

(In reply to comment #38)
> The commit queue is complaining because your ChangeLog entry in Source/WebKit/chromium/ChangeLog is malformed, not because you're lacking a reviewer.  Add back the "Reviewed by NOBODY (OOPS!)." line

Dammit - I must have dropped that merging changes between branches.  Thanks for catching it.  Can I get a cq+ again please?  Only change is adding that line to the Changelog.

[WebKit Review Bot]

Comment on attachment 164432 [details]
Fix ChangeLog

Clearing flags on attachment: 164432

Committed r128794: <http://trac.webkit.org/changeset/128794>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.