Bug 102823 – [chromium] Link highlight should display for at least 100ms, and fadeout on GestureTap/Longpress.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Link highlight should display for at least 100ms, and fadeout on GestureTap/Longpress.

[W. James MacLean]

Created attachment 175241 [details]
Patch

[Adrienne Walker]

Comment on attachment 175241 [details]
Patch

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

> Source/WebKit/chromium/src/LinkHighlight.cpp:70
> +    , m_startTime(monotonicallyIncreasingTime())

What are the causes of having a lot of time between LinkHighlight's ctor and startHighlightAnimationIfNeeded, causing the duration to be short?

> Source/WebKit/chromium/src/LinkHighlight.cpp:264
> +        curve->add(WebFloatKeyframe(minDuration - durationThusFar, startOpacity));

I don't understand only conditionalizing this one line.  So previously the animation would wait for half the duration and then linearly fade from startOpacity -> 0.  But now it seems like if it takes a while for the highlight to start, then it will fade out really quickly, which seems odd.

[W. James MacLean]

Comment on attachment 175241 [details]
Patch

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

>> Source/WebKit/chromium/src/LinkHighlight.cpp:70
>> +    , m_startTime(monotonicallyIncreasingTime())
> 
> What are the causes of having a lot of time between LinkHighlight's ctor and startHighlightAnimationIfNeeded, causing the duration to be short?

If a GestureTapDown and a GestureTap are generated in quick succession, e.g. a user taps quickly and lightly, then the link highlight just flashes really briefly and disappears. (GestureTap used to lead to the highlight being cleared, and not faded out, which it seems is too abrupt.)

>> Source/WebKit/chromium/src/LinkHighlight.cpp:264
>> +        curve->add(WebFloatKeyframe(minDuration - durationThusFar, startOpacity));
> 
> I don't understand only conditionalizing this one line.  So previously the animation would wait for half the duration and then linearly fade from startOpacity -> 0.  But now it seems like if it takes a while for the highlight to start, then it will fade out really quickly, which seems odd.

It is possible for the duration to be (much) longer than the minDuration (e.g. the user touches the screen and holds their finger there - in this case the highlight should remain displayed as long as they hold their finger down, or until GestureLongPress kicks in). In this case, if we're already over the minDuration, then we just start a 100 mS fadeout.

This CL is mostly about adjusting (tweaking?) the feel. Originally, the duration was fixed and much longer, so we (arbitrarily) decided to have constant opacity until the halfway point then start to fadeout. The current desire is to guarantee at least 100 mS of full display, followed by 100 mS of fadeout. The fast fadeout is comfortable for the users who have tried it, but it use to be with GestureLongPress or GestureTap that the highlight could display for a very short time, and just look like a flash.

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 175241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175241&action=review
> 
> >> Source/WebKit/chromium/src/LinkHighlight.cpp:70
> >> +    , m_startTime(monotonicallyIncreasingTime())
> > 
> > What are the causes of having a lot of time between LinkHighlight's ctor and startHighlightAnimationIfNeeded, causing the duration to be short?
> 
> If a GestureTapDown and a GestureTap are generated in quick succession, e.g. a user taps quickly and lightly, then the link highlight just flashes really briefly and disappears. (GestureTap used to lead to the highlight being cleared, and not faded out, which it seems is too abrupt.)

Ah, I see.  I think I misunderstood before.  Am I right that the start time is when the link highlight first appears, not when the fade out animation starts?

> >> Source/WebKit/chromium/src/LinkHighlight.cpp:264
> >> +        curve->add(WebFloatKeyframe(minDuration - durationThusFar, startOpacity));
> > 
> > I don't understand only conditionalizing this one line.  So previously the animation would wait for half the duration and then linearly fade from startOpacity -> 0.  But now it seems like if it takes a while for the highlight to start, then it will fade out really quickly, which seems odd.
> 
> It is possible for the duration to be (much) longer than the minDuration (e.g. the user touches the screen and holds their finger there - in this case the highlight should remain displayed as long as they hold their finger down, or until GestureLongPress kicks in). In this case, if we're already over the minDuration, then we just start a 100 mS fadeout.
> 
> This CL is mostly about adjusting (tweaking?) the feel. Originally, the duration was fixed and much longer, so we (arbitrarily) decided to have constant opacity until the halfway point then start to fadeout. The current desire is to guarantee at least 100 mS of full display, followed by 100 mS of fadeout. The fast fadeout is comfortable for the users who have tried it, but it use to be with GestureLongPress or GestureTap that the highlight could display for a very short time, and just look like a flash.

I'm trying to say that it doesn't look like the code as-is guarantees 100 ms of fadeout.  If you start fading almost immediately, then durationThusFar = epsilon and you add a key for t=(endTime - epsilon) value=startOpacity and t=endTime value=0, which seems like you have only have a fadeout over a duration of length epsilon.  Do you need to move the end time as well?

[W. James MacLean]

Comment on attachment 175241 [details]
Patch

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

>>>> Source/WebKit/chromium/src/LinkHighlight.cpp:70
>>>> +    , m_startTime(monotonicallyIncreasingTime())
>>> 
>>> What are the causes of having a lot of time between LinkHighlight's ctor and startHighlightAnimationIfNeeded, causing the duration to be short?
>> 
>> If a GestureTapDown and a GestureTap are generated in quick succession, e.g. a user taps quickly and lightly, then the link highlight just flashes really briefly and disappears. (GestureTap used to lead to the highlight being cleared, and not faded out, which it seems is too abrupt.)
> 
> Ah, I see.  I think I misunderstood before.  Am I right that the start time is when the link highlight first appears, not when the fade out animation starts?

Yes, when the highlight first appears.

>>>> Source/WebKit/chromium/src/LinkHighlight.cpp:264
>>>> +        curve->add(WebFloatKeyframe(minDuration - durationThusFar, startOpacity));
>>> 
>>> I don't understand only conditionalizing this one line.  So previously the animation would wait for half the duration and then linearly fade from startOpacity -> 0.  But now it seems like if it takes a while for the highlight to start, then it will fade out really quickly, which seems odd.
>> 
>> It is possible for the duration to be (much) longer than the minDuration (e.g. the user touches the screen and holds their finger there - in this case the highlight should remain displayed as long as they hold their finger down, or until GestureLongPress kicks in). In this case, if we're already over the minDuration, then we just start a 100 mS fadeout.
>> 
>> This CL is mostly about adjusting (tweaking?) the feel. Originally, the duration was fixed and much longer, so we (arbitrarily) decided to have constant opacity until the halfway point then start to fadeout. The current desire is to guarantee at least 100 mS of full display, followed by 100 mS of fadeout. The fast fadeout is comfortable for the users who have tried it, but it use to be with GestureLongPress or GestureTap that the highlight could display for a very short time, and just look like a flash.
> 
> I'm trying to say that it doesn't look like the code as-is guarantees 100 ms of fadeout.  If you start fading almost immediately, then durationThusFar = epsilon and you add a key for t=(endTime - epsilon) value=startOpacity and t=endTime value=0, which seems like you have only have a fadeout over a duration of length epsilon.  Do you need to move the end time as well?

D'oh, you're right ... missed that! +1 for code review. New patch shortly ;-)

[W. James MacLean]

Created attachment 175263 [details]
Patch

[Adrienne Walker]

Comment on attachment 175263 [details]
Patch

R=me.  This looks good to me, although I really wish we could have some unit tests for this sort of change.

[W. James MacLean]

Created attachment 175423 [details]
Patch for landing

[W. James MacLean]

(In reply to comment #7)
> (From update of attachment 175263 [details])
> R=me.  This looks good to me, although I really wish we could have some unit tests for this sort of change.

I'll try to put together a unit test for this today.

[WebKit Review Bot]

Comment on attachment 175423 [details]
Patch for landing

Clearing flags on attachment: 175423

Committed r135403: <http://trac.webkit.org/changeset/135403>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.