Bug 97355 – [chromium] Modify gesture highlight behaviour. Cancel on GestureLongPress and animate on GestureTapCancel.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Modify gesture highlight behaviour. Cancel on GestureLongPress and animate on GestureTapCancel.

[W. James MacLean]

Created attachment 165178 [details]
Patch

[Adrienne Walker]

Comment on attachment 165178 [details]
Patch

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

> Source/WebKit/chromium/src/LinkHighlight.cpp:-79
> -    // We don't want to show the highlight until startAnimation is called, even though the highlight
> -    // layer may be added to the tree immediately.

Why is this comment no longer true? What has changed?

> Source/WebKit/chromium/src/WebViewImpl.cpp:747
> +        fprintf(stderr, "wjm: GestureLongPress event.\n");

...

> Source/WebKit/chromium/src/WebViewImpl.cpp:779
> +        if (m_linkHighlight && !m_linkHighlight->isAnimating())
> +            m_linkHighlight->startHighlightAnimation();

This is just a small nit, but it almost seems like startHighlightAnimation() should internally make it impossible to call multiple times.  For example, if it were startHighlightAnimationIfNeeded, then callers don't have to know about isAnimating or not and can't screw it up.

> Source/WebKit/chromium/src/WebViewImpl.cpp:787
> +        if (m_linkHighlight && !m_linkHighlight->isAnimating())
> +            m_linkHighlight->startHighlightAnimation();

Are you maybe missing some "break" statements here in this switch statement? Surely GesturePinchBegin should not start and then immediately clear the link highlight?

[W. James MacLean]

Comment on attachment 165178 [details]
Patch

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

>> Source/WebKit/chromium/src/LinkHighlight.cpp:-79
>> -    // layer may be added to the tree immediately.
> 
> Why is this comment no longer true? What has changed?

In the old behaviour, we started the animation right away, and used this to display the highlight. In the new behaviour, we show the highlight right away, and do the following:
- as long as the user holds their finger down, the highlight remains
- after a delay GestureLongPress is received, cancelling the highlight, *or*
- the user moves their finger to initiate GestureScrollBegin, and the highlight animates away, *or*
- the use lifts their finger, completing GestureTap and the highlight disappears (we may or may not navigate away at this point).

It used to be that that there was a delay in adding the highlight to the tree, then the animation started a brief time later, giving us time to cancel it before it ever appeared.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:747
>> +        fprintf(stderr, "wjm: GestureLongPress event.\n");
> 
> ...

D'oh! Removed ...

>> Source/WebKit/chromium/src/WebViewImpl.cpp:779
>> +            m_linkHighlight->startHighlightAnimation();
> 
> This is just a small nit, but it almost seems like startHighlightAnimation() should internally make it impossible to call multiple times.  For example, if it were startHighlightAnimationIfNeeded, then callers don't have to know about isAnimating or not and can't screw it up.

Yeah, I considered this, wasn't sure, and flipped a coin. I like the name change though - fixed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:787
>> +            m_linkHighlight->startHighlightAnimation();
> 
> Are you maybe missing some "break" statements here in this switch statement? Surely GesturePinchBegin should not start and then immediately clear the link highlight?

Hmm, yes, this is muddled. Fixed in new patch.

[W. James MacLean]

Created attachment 167008 [details]
Patch

[Adrienne Walker]

Comment on attachment 167008 [details]
Patch

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

Why are the tests in chromium-linux here too? Are there no link highlights on other platforms?

> Source/WebKit/chromium/src/WebViewImpl.cpp:744
> +        fprintf(stderr, "wjm: GestureTapDown event.\n");

...

> Source/WebKit/chromium/src/WebViewImpl.cpp:766
> +            if (event.type == WebInputEvent::GestureScrollBegin || event.type == WebInputEvent::GestureTapCancel)

Can I suggest pulling out all the link highlight behavior into its own switch statement where you just have a list of events that should fade out the highlight and a list of events that should clear the highlight, all in one place.

This giant switch statement is hard enough to read as it is, and the gesture -> link highlight behavior mapping is undertested.  I worry about somebody in the future adding a new gesture to this switch statement that might accidentally and incorrectly start cancelling link highlights.

[W. James MacLean]

(In reply to comment #5)
> (From update of attachment 167008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167008&action=review
> 
> Why are the tests in chromium-linux here too? Are there no link highlights on other platforms?

At present this is just used on ChromeOS (linux + touch-enabled). Once behaviour is vetted here, we will add Win 8 into the mix.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:744
> > +        fprintf(stderr, "wjm: GestureTapDown event.\n");
> 
> ...
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:766
> > +            if (event.type == WebInputEvent::GestureScrollBegin || event.type == WebInputEvent::GestureTapCancel)
> 
> Can I suggest pulling out all the link highlight behavior into its own switch statement where you just have a list of events that should fade out the highlight and a list of events that should clear the highlight, all in one place.
> 
> This giant switch statement is hard enough to read as it is, and the gesture -> link highlight behavior mapping is undertested.  I worry about somebody in the future adding a new gesture to this switch statement that might accidentally and incorrectly start cancelling link highlights.

I like the sound of that, will revise and upload a new patch.

[W. James MacLean]

Created attachment 167351 [details]
Patch

[Adrienne Walker]

Comment on attachment 167351 [details]
Patch

R=me.  Thanks for those changes.  I feel like that's way easier to understand.

[W. James MacLean]

Comment on attachment 167351 [details]
Patch

Agreed, they do make it easier to read ... thanks!

[WebKit Review Bot]

Comment on attachment 167351 [details]
Patch

Clearing flags on attachment: 167351

Committed r130539: <http://trac.webkit.org/changeset/130539>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.