Bug 96060

Summary: Trigger node active state with touch gesture events
Product: WebKit Reporter: Rick Byers <rbyers>
Component: UI EventsAssignee: Rick Byers <rbyers>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aelias, allan.jensen, dglazkov, fishd, jamesr, rjkroege, tkent+wkapi, tonikitoo, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96183, 96788    
Bug Blocks: 96677    
Attachments:
Description Flags
Patch
none
Combined with 96183 for EWS
none
Add new layout test and updated expectations
none
Fixes, and opt-in using platform ifdefs per jamesr
none
Add iframe-based layout test
none
Patch
none
Merge with trunk
none
Tweaks based on CR feedback
none
Fix ChangeLog none

Description Rick Byers 2012-09-06 20:59:29 PDT
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.
Comment 1 Rick Byers 2012-09-08 19:17:15 PDT
Created attachment 162974 [details]
Patch
Comment 2 Rick Byers 2012-09-08 19:18:59 PDT
Created attachment 162975 [details]
Combined with 96183 for EWS
Comment 3 WebKit Review Bot 2012-09-08 19:20:24 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Rick Byers 2012-09-08 20:35:17 PDT
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).
Comment 5 WebKit Review Bot 2012-09-09 03:34:47 PDT
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
Comment 6 James Robinson 2012-09-09 16:24:13 PDT
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?
Comment 7 Rick Byers 2012-09-09 18:01:55 PDT
(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?
Comment 8 Rick Byers 2012-09-11 14:27:01 PDT
Created attachment 163438 [details]
Add new layout test and updated expectations
Comment 9 WebKit Review Bot 2012-09-11 16:13:58 PDT
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
Comment 10 Rick Byers 2012-09-11 16:33:58 PDT
Created attachment 163476 [details]
Fixes, and opt-in using platform ifdefs per jamesr
Comment 11 Rick Byers 2012-09-11 19:25:47 PDT
Created attachment 163498 [details]
Add iframe-based layout test
Comment 12 Rick Byers 2012-09-12 18:53:15 PDT
Comment on attachment 163498 [details]
Add iframe-based layout test

Ping - who is the right person to review this change?  ABarth perhaps?
Comment 13 Rick Byers 2012-09-13 11:22:52 PDT
CC allan.jensen since he's actively changing related code (updateHoverActiveState, hit testing)
Comment 14 Adam Barth 2012-09-13 15:07:21 PDT
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.
Comment 15 James Robinson 2012-09-13 15:25:17 PDT
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?
Comment 16 Allan Sandfeld Jensen 2012-09-13 15:52:54 PDT
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.
Comment 17 Rick Byers 2012-09-13 16:04:04 PDT
(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?
Comment 18 Rick Byers 2012-09-13 16:11:43 PDT
(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).
Comment 19 Allan Sandfeld Jensen 2012-09-14 04:47:17 PDT
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?
Comment 20 Allan Sandfeld Jensen 2012-09-14 04:48:28 PDT
(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 :)
Comment 21 Antonio Gomes 2012-09-14 05:25:40 PDT
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.
Comment 22 Rick Byers 2012-09-14 07:16:44 PDT
(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?
Comment 23 Rick Byers 2012-09-14 07:19:06 PDT
(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.
Comment 24 Rick Byers 2012-09-14 08:04:07 PDT
(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).
Comment 25 Rick Byers 2012-09-14 08:36:31 PDT
Created attachment 164162 [details]
Patch
Comment 26 Rick Byers 2012-09-14 08:38:22 PDT
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?
Comment 27 Rick Byers 2012-09-14 10:25:33 PDT
Created attachment 164183 [details]
Merge with trunk
Comment 28 Rick Byers 2012-09-14 10:28:42 PDT
Comment on attachment 164183 [details]
Merge with trunk

This has been merged with the fix for 96788.
Comment 29 Rick Byers 2012-09-16 16:58:52 PDT
Ping - Antonio, any other concerns/questions?
Comment 30 Antonio Gomes 2012-09-17 05:10:54 PDT
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?
Comment 31 Rick Byers 2012-09-17 09:16:33 PDT
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.
Comment 32 Allan Sandfeld Jensen 2012-09-17 09:22:39 PDT
(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.
Comment 33 WebKit Review Bot 2012-09-17 09:32:28 PDT
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
Comment 34 Rick Byers 2012-09-17 09:33:26 PDT
Created attachment 164408 [details]
Tweaks based on CR feedback
Comment 35 Rick Byers 2012-09-17 09:35:13 PDT
(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.
Comment 36 WebKit Review Bot 2012-09-17 09:38:53 PDT
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
Comment 37 Rick Byers 2012-09-17 10:05:09 PDT
(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).
Comment 38 James Robinson 2012-09-17 11:01:57 PDT
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
Comment 39 Rick Byers 2012-09-17 12:02:13 PDT
Created attachment 164432 [details]
Fix ChangeLog
Comment 40 Rick Byers 2012-09-17 12:04:47 PDT
(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.
Comment 41 WebKit Review Bot 2012-09-17 12:39:17 PDT
Comment on attachment 164432 [details]
Fix ChangeLog

Clearing flags on attachment: 164432

Committed r128794: <http://trac.webkit.org/changeset/128794>
Comment 42 WebKit Review Bot 2012-09-17 12:39:23 PDT
All reviewed patches have been landed.  Closing bug.