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.

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

Jump to bottom

fix(material/button): cdk-focus classes not being applied #25619

Merged
merged 1 commit into from Sep 30, 2022
Merged

fix(material/button): cdk-focus classes not being applied #25619

merged 1 commit into from Sep 30, 2022

Conversation

kevinjihwanlee
Copy link
Contributor

Fixes a bug in the Angular Material button component where cdk-focused, cdk-keyboard-focused, cdk-mouse-focused were not being applied upon respective focus. This is because there was no FocusManager set up with the MDC buttons like the legacy button did.

Fixes #25618

crisbeto
crisbeto reviewed Sep 10, 2022
View reviewed changes
src/material/button/button-base.ts Outdated
@@ -95,6 +103,7 @@ export class MatButtonBase
elementRef: ElementRef,
public _platform: Platform,
public _ngZone: NgZone,
private _focusMonitor: FocusMonitor,
Copy link
Member

Choose a reason for hiding this comment

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

You should use the new inject function to get the focus monitor. That way you can avoid all the constructor changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point - done.

src/material/button/button-base.ts Outdated
@@ -95,6 +103,7 @@ export class MatButtonBase
elementRef: ElementRef,
public _platform: Platform,
public _ngZone: NgZone,
private _focusMonitor: FocusMonitor,
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the focus monitor styles for the focus styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to these styles here: https://github.com/angular/components/blob/main/src/material/legacy-button/_button-base.scss#L67 I believe that should be the only one for the button component. Visually I can't tell the difference between having those styles or not, but maybe they were included with the legacy button for a reason.

Copy link
Member

Choose a reason for hiding this comment

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

Those are the focus styles for the legacy button. I was referring to these https://github.com/angular/components/blob/main/src/material/button/_button-theme-private.scss#L21.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, added .cdk-focused - would that suffice?

Copy link
Member

Choose a reason for hiding this comment

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

I think that we should match the behavior of the old button where the focus indication is only shown for keyboard and programmatic focus. Also the :focus selector is now redundant and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - done.

@kevinjihwanlee kevinjihwanlee force-pushed the cdk-focus-mdc-button branch 4 times, most recently from af91153 to 6622bc1 Compare September 10, 2022 07:12
@crisbeto
Copy link
Member

The code LGTM, but one of the CI checks is failing. You can run yarn approve-api button and commit the changed file to fix it.

crisbeto
crisbeto reviewed Sep 10, 2022
View reviewed changes
src/material/button/_button-theme-private.scss Outdated
@@ -18,8 +18,11 @@
opacity: map.get($opacities, hover);
}

&:focus .mat-mdc-button-persistent-ripple::before {
opacity: map.get($opacities, focus);
.cdk-program-focused,
Copy link
Member

Choose a reason for hiding this comment

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

Ah actually these selectors should be prefixed with &, otherwise the button focus indication will show when it's inside of any focused element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - done.

@kevinjihwanlee
fix(material/button): cdk-focus classes not being applied
f2059c0 
Fixes a bug in the Angular Material `button` component where
cdk-focused, cdk-keyboard-focused, cdk-mouse-focused were
not being applied upon respective focus. This is because
there was no FocusManager set up with the MDC buttons like
the legacy button did.

Fixes angular#25618
crisbeto
crisbeto approved these changes Sep 10, 2022
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Sep 10, 2022
@andrewseguin andrewseguin self-assigned this Sep 22, 2022
@devversion devversion removed their request for review September 24, 2022 16:13
@andrewseguin andrewseguin added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 30, 2022
@andrewseguin andrewseguin merged commit 9f0071d into angular:main Sep 30, 2022
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin andrewseguin is a code owner

@josephperrott josephperrott Awaiting requested review from josephperrott

Assignees

@andrewseguin andrewseguin

Labels
action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug(Button): cdk-focus classes not being applied
3 participants
@kevinjihwanlee @crisbeto @andrewseguin