Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

rudzikdawid
Copy link
Contributor

@rudzikdawid rudzikdawid commented Aug 17, 2018

Closes #8518

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

after focus on switch component using keyboard we can see dark ripple ink on dark theme

light theme before:

dark theme before:

Issue Number: #8518

What is the new behavior?

after focus on switch component using keyboard we can see bright ripple ink on dark theme

light theme after:

dark theme after:

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Aug 17, 2018
@Splaktar Splaktar self-requested a review August 25, 2018 00:53
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this!

I did some manual testing and reviewing against the spec. I've left some comments on how we can better align with the spec.

background-color: '{{background-500}}';
}

&.md-focused:not([disabled]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on https://material.io/archive/guidelines/components/selection-controls.html#selection-controls-switch, it seems like the thumb needs to be displayed when focused and disabled. This can't be done with tabbing, but it is possible to focus a disabled switch when using a screen reader or other assistive device.

We should probably remove this disabled check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some more testing and the following seemed to work best for me:

  &.md-focused {
    &:not(.md-checked) {
      .md-thumb:before {
        background-color: '{{foreground-4}}';
      }
    }
    &[disabled] {
      .md-thumb:before {
        background-color: '{{foreground-4}}';
      }
    }
  }

}
}

&.md-checked {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add :not([disabled]) to this rule so that disabled, focused switches don't get colored.

.md-thumb:before {
left: -8px;
top: -8px;
right: -8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to remove :not([disabled]) from this rule as well so that the circle has a proper size when disabled switches are focused.

@Splaktar Splaktar added this to the 1.1.11 milestone Aug 25, 2018
@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Aug 25, 2018
@Splaktar Splaktar self-assigned this Aug 25, 2018
@Splaktar Splaktar added type: bug ui: theme P4: minor Minor issues. May not be fixed without community contributions. labels Aug 25, 2018
@Splaktar
Copy link
Contributor

@rudzikdawid I'd like to get this into the 1.1.11 release that is coming out soon. Can you please look at the PR review feedback and let me know if you can address it this week?

@Splaktar
Copy link
Contributor

@rudzikdawid I'm sure that you are busy, so I made the changes and opened PR #11459 with both your changes and the ones I suggested above.

Can you please post in that PR that you approve the use of your commit so that the caretakers will be able to mark the CLA: yes?

@Splaktar Splaktar closed this Sep 25, 2018
@Splaktar Splaktar removed this from the 1.1.11 milestone Sep 25, 2018
@Splaktar Splaktar removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 25, 2018
jelbourn pushed a commit that referenced this pull request Nov 6, 2018
marosoft pushed a commit to marosoft/material that referenced this pull request Nov 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P4: minor Minor issues. May not be fixed without community contributions. type: bug ui: theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

switch: focus indicator is wrong on dark themes
3 participants