New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 595315 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Feature

Blocking:
issue 458662


Participants' hotlists:
Fixing-touch


Sign in to add a comment

Ripple animations should not be shown on touch input for Windows

Project Member Reported by tdander...@chromium.org, Mar 16 2016

Issue description

Windows has its own OS-level feedback effect on touch input, so do not show the material design ripple effects on buttons in response to touch.

 
Labels: -M-51 M-52
Blocking: 458662
Labels: -Pri-1 -M-52 M-53 Pri-2
Owner: bruthig@chromium.org
I started investigating this last night, stay tuned for findings.
Status: Started (was: Available)
Owner: ----
Status: Available (was: Started)
Now that https://codereview.chromium.org/2042073002/ has landed it should be much easier to complete this issue.  All ripple animations now go through InkDropHostView::AnimateInkDrop() which accepts a 'const ui::LocatedEvent*' parameter.  So it should be easy to use an '#if defined(OS_WIN)' and suppress ripple animations based on the |event| parameter.

The tricky part is going to be making sure the ripples still make valid state transitions with the introduction of gesture events not triggering animations.
e.g.
1. User presses and holds left mouse button on 'Home' button => ripple animates to ACTION_PENDING
2. User taps the 'Home' button => triggers Home (this should definitely make the ripple disappear, probably via a ACTION_TRIGGERED animation)
Owner: kylixrd@chromium.org
Status: Assigned (was: Available)
Could it really be as simple as adding this to InkDropHostView::AnimateInkDrop()?

#if defined(OS_WIN)
  if (state != InkDropState::HIDDEN && event && event->IsGestureEvent())
    return;
#endif

I did this and tried the steps in #5 and it seems to work as intended.
I think it will be slightly more complex than that.
Some examples to think through are:
1. The ACTIVATED animation should occur no matter what the event type is.
2. An ACTION_TRIGGERED, ALT_ACTION_TRIGGERED and DEACTIVATED animation triggered by a gesture event should probably happen if the ripple is already visible.
3. And we should try and make sure ripples are robust when multiple input sources are interacting with the button.  This is probably a small use case but leaving a ripple persist is a pretty bad user experience.
Ok, after looking over the available InkDropStates and your comment in #8, it seems that we really only want to block any "pending" actions from touch gestures. The rest of them should go ahead and pass on through.

#if defined(OS_WIN)
  if ((state == InkDropState::PENDING ||
      state == InkDropState::ALTERNATE_ACTION_PENDING) && event &&
      event->IsGestureEvent())
    return;
#endif

When I do that, it looks odd. I see the remnants of the OS ink-drop and the action animation...

This is on a Surface Book with Windows 10. I'll see if I can make a short video and post it.
The built-in screen/app recorder in Win10 doesn't capture the OS ink-drops... only the ink-drops we paint. This makes it hard to show what I'm seeing here.
Maybe high frame rate recording with a phone might work.
Certainly a little more complicated, but this seems to work.

#if defined(OS_WIN)
  if ((state == InkDropState::ACTION_PENDING ||
       state == InkDropState::ALTERNATE_ACTION_PENDING ||
       ((state == InkDropState::ACTION_TRIGGERED ||
         state == InkDropState::ALTERNATE_ACTION_TRIGGERED) &&
        ink_drop_->GetTargetInkDropState() == InkDropState::HIDDEN)) &&
      event && event->IsGestureEvent())
    return;
#endif

When it lands, can you also verify that your change papers over  bug 615013 ?
Project Member

Comment 15 by bugdroid1@chromium.org, Jun 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d480e04cbde45c22342599468c3081f586b4523c

commit d480e04cbde45c22342599468c3081f586b4523c
Author: kylixrd <kylixrd@chromium.org>
Date: Mon Jun 27 18:48:36 2016

Don't show ink drops with touch events on Windows

BUG= 595315 
TBR=bruthig@chromium.org

Review-Url: https://codereview.chromium.org/2095873004
Cr-Commit-Position: refs/heads/master@{#402239}

[modify] https://crrev.com/d480e04cbde45c22342599468c3081f586b4523c/ui/views/animation/ink_drop_host_view.cc

Just verified with today's Canary build on my Surface Book. No highlight remains or is shown for a long-tap or touch.
Status: Verified (was: Assigned)
Project Member

Comment 19 by bugdroid1@chromium.org, Jul 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82397d9c36b76e2869ab77ede2df736871c752b2

commit 82397d9c36b76e2869ab77ede2df736871c752b2
Author: kelvinp <kelvinp@chromium.org>
Date: Fri Jul 01 16:32:16 2016

Revert of Added tests and reworked ink-drop touch logic (patchset #2 id:20001 of https://codereview.chromium.org/2117603002/ )

Reason for revert:
Compilation failure on Chromium.win builder.

https://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder/builds/22983/steps/compile/logs/stdio

Original issue's description:
> Added tests and reworked ink-drop touch logic
>
> BUG= 595315 
>
> Committed: https://crrev.com/fff99399f7fcfe17950174897fb47a26f6047c78
> Cr-Commit-Position: refs/heads/master@{#403462}

TBR=bruthig@chromium.org,kylixrd@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 595315 

Review-Url: https://codereview.chromium.org/2115933002
Cr-Commit-Position: refs/heads/master@{#403466}

[modify] https://crrev.com/82397d9c36b76e2869ab77ede2df736871c752b2/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/82397d9c36b76e2869ab77ede2df736871c752b2/ui/views/animation/ink_drop_host_view_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Jul 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2492af4d898c22ca3a341e67cb223e0e7ebabe0c

commit 2492af4d898c22ca3a341e67cb223e0e7ebabe0c
Author: kylixrd <kylixrd@chromium.org>
Date: Fri Jul 01 18:02:28 2016

Added tests and reworked ink-drop touch logic

BUG= 595315 

Committed: https://crrev.com/fff99399f7fcfe17950174897fb47a26f6047c78
Review-Url: https://codereview.chromium.org/2117603002
Cr-Original-Commit-Position: refs/heads/master@{#403462}
Cr-Commit-Position: refs/heads/master@{#403481}

[modify] https://crrev.com/2492af4d898c22ca3a341e67cb223e0e7ebabe0c/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/2492af4d898c22ca3a341e67cb223e0e7ebabe0c/ui/views/animation/ink_drop_host_view_unittest.cc

Labels: Merge-Request-53
https://codereview.chromium.org/2117603002/
Owner: bruthig@chromium.org

Comment 23 by dimu@google.com, Jul 6 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 24 by bugdroid1@chromium.org, Jul 7 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95e3c7a8876a78816484f446682b2ec8d87d3760

commit 95e3c7a8876a78816484f446682b2ec8d87d3760
Author: Ben Ruthig <bruthig@chromium.org>
Date: Thu Jul 07 18:11:24 2016

Added tests and reworked ink-drop touch logic

BUG= 595315 

Review-Url: https://codereview.chromium.org/2117603002
Cr-Commit-Position: refs/heads/master@{#403462}
(cherry picked from commit fff99399f7fcfe17950174897fb47a26f6047c78)

Review URL: https://codereview.chromium.org/2125353002 .

Cr-Commit-Position: refs/branch-heads/2785@{#45}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/95e3c7a8876a78816484f446682b2ec8d87d3760/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/95e3c7a8876a78816484f446682b2ec8d87d3760/ui/views/animation/ink_drop_host_view_unittest.cc

Components: UI>Shell>TouchView
Components: -UI>Touch

Sign in to add a comment