[chromium] Link highlight should display for at least 100ms, and fadeout on GestureTap/Longpress.
Created attachment 175241 [details] Patch
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.
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.
(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?
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 ;-)
Created attachment 175263 [details] Patch
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.
Created attachment 175423 [details] Patch for landing
(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.
Comment on attachment 175423 [details] Patch for landing Clearing flags on attachment: 175423 Committed r135403: <http://trac.webkit.org/changeset/135403>
All reviewed patches have been landed. Closing bug.