Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ripple): Deactivate on contextmenu event #3759

Merged
merged 2 commits into from
Oct 15, 2018

Conversation

kfranqueiro
Copy link
Contributor

Fixes #3524.

While testing I wasn't able to get tap-hold to bring up the context menu at all on mobile for some reason... but I confirmed that this fixes the issue for right-click in Chrome and Safari on OS X, which are the only browsers I could reproduce it on.

OS Browser Before After
OS X Chrome Buggy OK
OS X Safari Buggy OK
OS X Firefox OK OK
Win10 Chrome OK OK
Win10 Firefox OK OK
Win10 Edge OK OK

Tested on Win10 with both mouse and touch. IE is N/A because it doesn't use JS for ripple effect.

@codecov-io
Copy link

codecov-io commented Oct 15, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@81bb919). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3759   +/-   ##
=========================================
  Coverage          ?   98.52%           
=========================================
  Files             ?      120           
  Lines             ?     5236           
  Branches          ?      658           
=========================================
  Hits              ?     5159           
  Misses            ?       77           
  Partials          ?        0
Impacted Files Coverage Δ
packages/mdc-ripple/foundation.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81bb919...94cf727. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 557 screenshot tests passed for commit a18b32e vs. master! 💯🎉

@williamernest
Copy link
Contributor

I'm not sure this is resolving all of the use cases where the ripple ends up stuck being activted. If you click on a button and then open a context menu on the button it ends up stuck again.

@kfranqueiro
Copy link
Contributor Author

I haven't been able to reproduce what you're describing, even if I purposely try opening the context menu before the first click's ripple activation finishes. The only state that stays for me is the focus state, which is expected.

Can you give full repro steps when you get a chance?

@williamernest
Copy link
Contributor

  1. Enable device toolbar and select a device (Pixel 2)
  2. Long press on button until context menu shows up. (I'm using baseline-button-with-icons.html)
  3. Click ESC to close context menu.
  4. Click Tab to go to next element.
    The ripple on the button remains in the activated state.

I was able to reproduce using these steps on OSX/Chrome. On Linux/Chrome substitute the following for step 2.

  1. Click on the button to activate the ripple.
  2. Right click on the button to show context menu.

@kfranqueiro
Copy link
Contributor Author

I've managed to reproduce this in Chrome + device emulation on desktop, but I think it's a device emulation quirk. I tested with demos/button.html with an added style to color :active elements red, and Chrome on desktop w/ device emulation seems to get the active state stuck when a long-press opens the context menu. Android Chrome and iOS Safari (simulated) don't seem to have any issue.

(GitHub doesn't allow attaching videos, but I've sent a few to Will outside of this thread.)

Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Issues only reproducible with the device emulator.

@mdc-web-bot
Copy link
Collaborator

All 557 screenshot tests passed for commit 94cf727 vs. master! 💯🎉

@kfranqueiro kfranqueiro merged commit 4d76e3f into master Oct 15, 2018
@kfranqueiro kfranqueiro deleted the fix/ripple-contextmenu-deactivate branch October 15, 2018 23:39
@jamesmfriedman jamesmfriedman mentioned this pull request Oct 30, 2018
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MDCRipple: Context menu causes ripple to remain activated
5 participants