Skip to content

Conversation

@chang47
Copy link
Contributor

@chang47 chang47 commented Aug 15, 2018

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

In the past, we explicitly put focus on the SplitButtonContainer in BaseButton when we do a menu click, because we don't want to be able to focus each individual buttons in a split button.

However this created a problem with SplitButtons in the event where we want to return focus to the previous element when we open a menu and hit escape to close it.

Specifically what happens is that we:

  1. Click on the menu
  2. Focus gets put on the split button container
  3. the previous element for the contextual menu is saved to the split button container.

As a result whenever we close the menu, focus will always return back to the split button container, which is great for all cases except for the one where we don't want this behavior.

To fix this without breaking existing behavior, I added a onFocusCapture event to the split button container and removed it from being able to be called when we hit the menu onclick event.

Through investigation, we found 2 keypoints with the out that a mouseDown event is that's important for this change:

  1. mouseDown is what focuses the element we clicked on
  2. mouseDown is required for us to fire the onFocus event

Here's the behavior If we click on the menu button in a split button:

  1. We fire the mouseDownEvent, this will put focus on the menu button that we clicked on
  2. The new onFocus event will be called and we will put manually put focus on the container
  3. The onClick event will happen normally to open the menu
  4. The menu will save the split button container as the previous element

Now the case for where we don't want to put focus on the split button container:

  1. We fire the mouseDownEvent, but (via the callback) we preventDefault() for the event, this will prevent the menu button from being focused
  2. Because we preventedDefault, the onFocus event will not be called and focus won't be put on the container
  3. The onClick will run as normal
  4. The menu will save wherever we were focused on as our previous element

By doing this, we get the new behavior we want where focus does not go back to the button and we still keep existing fabric behavior.

Focus areas to test

I've validated that in both buttons and split buttons:

  1. If the user focus on the buttons via keyboard and opens the menu either via alt + down or enter and then close the menu, focus would still remain in the button.
  2. If the user opens a menu via a click and presses escape to close it, focus will focus on the split button container.

In the case where we do a preventDefault() for the mouseDown event:

  1. If the user focus on the buttons via keyboard and opens the menu either via alt + down or enter and then close the menu, focus would still remain in the button.
    2. If the user opens a menu via a click and press escape, focus now goes back to wherever we were focusing on prior to clicking on the button.
Microsoft Reviewers: Open in CodeFlow

@chang47 chang47 requested a review from jspurlin August 15, 2018 02:23
@chang47 chang47 changed the title Chiechan/stop agressive focus in menus BaseButton: Remove us unncessary putting focus on the split button we toggle a menu Aug 15, 2018
@chang47 chang47 changed the title BaseButton: Remove us unncessary putting focus on the split button we toggle a menu BaseButton: Remove unnecessary putting focus on the split button we toggle a menu Aug 15, 2018
private _onToggleMenu = (shouldFocusOnContainer: boolean): void => {
if (this._splitButtonContainer.current) {
this._splitButtonContainer.current.focus();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right fix? It looks like you added it in March to fix some specific issues: #4222

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like we talked offline, we're getting rid of this and instead we're using the onFocus capture event to ensure that focus is set to the SplitButton Container instead of us manually putting it there.

REDMOND\chiechan added 3 commits August 15, 2018 16:34
@chang47 chang47 requested a review from joschect as a code owner August 16, 2018 18:52
@chang47 chang47 changed the title BaseButton: Remove unnecessary putting focus on the split button we toggle a menu "BaseButton: Add onFocusCapture to the split button container to focus the container instead of doing it in the menu onClick Aug 17, 2018
@jspurlin
Copy link
Contributor

@chang47 for #2 of your areas to test, by default we want the focus to go back to the button when the menu is closed, correct? A creator can change that (by preventingDefault on mouseDown, for example)

@chang47
Copy link
Contributor Author

chang47 commented Aug 17, 2018

@jspurlin Yes, I'll edit the validation to be more clear.

@chang47 chang47 merged commit 16b26d2 into microsoft:master Aug 20, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants