Skip to content

fix(slide-toggle): don't show hover ripples on touch devices #13702

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 23, 2018
Merged

fix(slide-toggle): don't show hover ripples on touch devices #13702

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/lib/slide-toggle/slide-toggle.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,16 @@ $mat-slide-toggle-bar-track-width: $mat-slide-toggle-bar-width - $mat-slide-togg
&, .mat-slide-toggle.mat-disabled .mat-slide-toggle-bar:hover & {
opacity: 0;
}

// Hover styles will be displayed after tapping on touch devices.
// Disable the hover styling if the user's device doesn't support hovering.
@media (hover: none) {
// Note that we only negate the `:hover` rather than setting it to always be `display: none`,
// in order to maintain the focus indication for hybrid touch + keyboard devices.
.mat-slide-toggle-bar:hover & {
display: none;
Copy link
Member

Choose a reason for hiding this comment

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

Why not just opacity: 0 as for the other selectors? Also looks like on touch devices the persistent ripple can be hidden without the :hover right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to prevent any selectors with a higher specificity from reintroducing the opacity.

Also looks like on touch devices the persistent ripple can be hidden without the :hover right?

I'm not sure I get it. Do you mean using the focus monitor classes?

Copy link
Member

Choose a reason for hiding this comment

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

I see, just feels a bit odd to use display: none because I personally just use display: none if something needs to be hidden for a longer time-span. But in this case, it can quickly toggle the display because you're only hiding the persistent ripple if the :hover pseudo element is activated.

I'm not sure I get it. Do you mean using the focus monitor classes?

I've meant that we could just set display: none permanently if @media (hover: none) is activated because in no other scenario on touch, the persistent ripple shows up (after #13562)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did initially (always having display: none), but I decided to keep the more specific selector in case the input receives keyboard focus somehow (e.g. programmatically through the FocusMonitor or by pressing the "Next" button on the virtual keyboard).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually thinking about it more, as it is now it allows us to keep the focus indication for hybrid devices that have both a touchscreen and a keyboard.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Can you add this into a comment? doesn't have to be too detailed.

}
}
}

/** Custom styling to make the slide-toggle usable in high contrast mode. */
Expand Down