fix(button): focus styles not applied to programmatically focused buttons

[crisbeto]

Currently the tint that is added on top of focused buttons won't show up if the button is focused programmatically, which means that the button won't appear focused in cases like the dialog closing and restoring focus to its trigger. This seems to have been introduced by 5d6920d.

Fixes #7510.

[mmalerba]

So it sounds like the motivation for this change was a specific issue with the dialog closing. In general for most cases do you think programmatic focus should default to looking like keyboard or like mouse? If keyboard, we should make this change. Otherwise we should leave the styles the same and change dialog to inject FocusOriginMonitor and use the focusVia method to pretend it was a keyboard focus, since it's more of an exception than the normal case. I don't have a strong preference on what the default should be, just wanted to point out the difference and make sure we make whatever change we make deliberately.

[crisbeto]

IMO all programmatic focus should look like the keyboard one, otherwise the user won't have any indication that something is focused. It's different for the mouse focus since the user is keeping track of their cursor and know what they clicked on so the indication isn't necessary.

[mmalerba]

I think the reason I originally made it act like mouse is because of the situation where you click a button to open a sidenav and then the sidenav opens and programmatically focuses its first element. We didn't want the focus styles on that first element. But maybe we can consider that more of the exception case and add some code to do focusVia.

Also if we're going to do this for button let's be consistent, can you search our CSS for cdk-keyboard-focused and make sure that we always put the same styles for cdk-program-focused?

[crisbeto]

Sure. I'll update this PR and ping you.

[crisbeto]

Updated the 3 other cases we were using it @mmalerba.

[mmalerba]

Can we change the sidenav to use focusVia so that we don't have a regressions with #3215? It's kinda tricky I think, because of the user opens the sidenav with the keyboard we want to show the focus style on the first element, but if they open it with the mouse we don't.

[crisbeto]

Done @mmalerba.

[mmalerba]

This is going to do the same thing that just calling .focus() was doing. I think we want logic like this:

1. If the sidenav close is triggered by keyboard interaction (e.g. pressing escape) set it to keyboard focused
2. If the sidenav close is triggered by mouse interaction (e.g. clicking on backdrop) set it to mouse focused
3. If the sidenav close is triggered by directly calling close or toggle method allow the user to optionally supply the focus origin and default to program if they don't specify.

I think opening the sidenav needs similar logic but it's a little trickier because the sidenav isn't directly responsible for setting the focus on open, it goes through the focus trap.

You can test if this is working properly by opening and closing the sidenav in the demo app via different means and seeing what the resulting focus effect looks like

[crisbeto]

I'm not sure how that would work, considering that we don't have the equivalent of mdMenuTriggerFor for the sidenav. Alternatively, I can fix this only for the dialog where we have more control over the closing logic.

[mmalerba]

I think the closing logic I described is fairly straightforward to implement. For open you can just allow the user to pass an origin to the open method and it will default to program if they don't specify. That way if people really care about getting rid of the focus indicator they can add logic for it manually. It might also be nice to make a mdSidenavTrigger directive that does the focus origin logic for them.

[crisbeto]

That makes sense. I was trying to think of something that would work automagically.

[crisbeto]

Reworked this one based on what we discussed a while ago @mmalerba, can you take another look?

[mmalerba]

can you update the demo app's sidenav as well, to avoid this bug:

1. click hamburger
2. click new demo page in sidenav
3. page loads & sidenav closes, observe focus style on hamburger menu after close (should not appear focused)

[crisbeto]

Done @mmalerba.

[angular-automatic-lock-bot]

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.}