Removing ability to focus with keyboarding on menu buttons for split buttons and and spin boxes #4222

chang47 · 2018-03-09T17:58:49Z

Problem:
Currently, in our spin buttons and our split buttons, we can keyboard to the menu button for split buttons and the increment/decrement button on the spin button if it's in a focus zone.

In terms of accessibility, they are an extra set of controls that are not needed. Instead focus should just be solely set on the the whole component.

Solution:
Spin button: added data-is-focusable=false for the the icon buttons so that we won't be able to focus on them while in a focus zone.

Split button:

Added tab indexing to the container div so we can focus on the whole container instead of the individual buttons.
Added data-is-focusable=false so we can avoid having the focus zone tabbing to the primary button/menu button and only the container.
Added the ability to call the primary button when we hit enter on the container.
Added disable styles

Contextual Menu Split Button:

Similar changes to the Split Button in base button, except I had to move some functions around to be more similar to base button.
Added focus styles .

Validation:
Spin button: focus no longer goes to the icon buttons in both focus zones and in normal situations when keyboarding.

Split Button: Same as spin button.

Contextual Menu: Same as spin button.

Pictures: Coming soon...

New changes with the contextual menu:

Old contextual menu split button:

New changes with a normal split button:

Old split button:

…only on the container

betrue-final-final · 2018-03-09T19:13:38Z

I recently rejected this in screener as it is breaking our Fabric focus.

chang47 · 2018-03-10T00:34:39Z

@jspurlin

jspurlin

@betrue-final-final how is this breaking focus in fabric?

chang47 · 2018-03-12T17:27:49Z

@jspurlin Before I didn't have the correct styling for the disabled split button (we were still outlining it), but I've since made the correct changes.

jspurlin · 2018-03-12T17:50:12Z

@chang47 it looks like the bundle maxSize needs to be updated from 44.7 to 44.9

jspurlin

@chang47 Can you include some gifs showing the old/new behavior?

chang47 · 2018-03-13T17:37:45Z

@jspurlin I added the before and after and I also updated the bundle size.

…react into focus2

dzearing · 2018-03-13T19:24:45Z

packages/office-ui-fabric-react/src/components/Button/examples/Button.Split.Example.tsx

              ]
            } }
          />
+        </div>        <div>


weird spacing

jspurlin · 2018-03-13T20:46:06Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+        onKeyDown={ this._onSplitContainerItemKeyDown.bind(this, item) }
+        onClick={ this._executeItemClick.bind(this, item) }
+        tabIndex={ 0 }
+        aria-hidden={ true }


This aria-hidden is incorrect, it should not be set on the button

Also, shouldn't this element have data-is-focusable="true"?

joschect · 2018-03-13T21:54:04Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

-          aria-hidden={ true }
-          className={ classNames.splitContainer }
-        >
+        <span aria-hidden={ true }>


since the flex classname is on the div rather than the span, that just means that the span will flex to fit, but there's only one element so it will always be able to fit. Should this span have been deleted? The contents of the span should likely still be flexed.

I put it back, because I wanted to have a way to have aria-hidden for all the child elements, but seeing as each of the pieces in the child element is uniquely created here, I don't see the harm in adding aria-hidden to each of the individual pieces.

Is there any reason why we aren't just putting the span back to the way it was before this change (the more scoped we make the change the better, and making this change doesn't seem necessary)?

joschect · 2018-03-13T21:55:20Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+    if (ev.which === KeyCodes.enter) {
+      this._executeItemClick(item, ev);
+    } else {
+      this._onItemKeyDown(item, ev);


I'm pretty sure you don't need this else as the event will continue to propagate to the next event handler.

That's an interesting point to discuss, if we press enter, on a button, that's the equivalent of pressing the onClick event, but should we also try to handle the onKeyDown event too? @jspurlin what do you think?

@chang47 you are correct, in this case (handling enter) we want to tie that with executeItemClick and not also fire onItemKeyDown

In the if statement you should prevent default and stop propagation, since it will propagate up to the keyhandler that's on the root unless you stop propagation.

joschect

Approved with suggestions.

dzearing · 2018-03-14T16:40:51Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx


-    return <BaseButton { ...splitButtonProps } onMouseDown={ this._onMouseDown } />;
-
+    return <BaseButton {...splitButtonProps} onMouseDown={ this._onMouseDown } tabIndex={ -1 } />;


Adding tabIndex is an antipattern here. If this is hosted in a focusZone, it will become focusable again.

Plus, if something should not be focusable, it shouldn't be rendered as a button.

My preference here is to convert the menu click target to a DIV or SPAN which can soak clicks and touches, but not receive focus, and not attempt to render as a BaseButton.

Setting the tabIndex=-1 and data-is-focusable=false ensures that the button will not be keyboard focusable when it is or is not inside of a FocusZone.

As for if the individual portions of the splitButton should be buttons, I'm working with Narrator on defining a splitButton so that they will always be able to tell that it is in fact a split button. One approach may be a button with x, y, z markup and the fact that it contains two buttons... We could change them to be divs or spans if we want, the only downside of that is now we lose the consistency when the splitButton is rendered in a ContextualMenu. We we want to update that as well then we'll lose the ability to use the default render functionality

That being said, I just noticed the span wrapping the splitButton contents here does not have aria-hidden="true" (which it should to align with how splitButtons are represented in ContextualMenus)

Chatted with @dzearing and we should go ahead and simplify this to not be created with two buttons

Talking with @jspurlin offline, we were discussing the idea of just removing the wrapping span. now that there is a single hittarget, there is no need for the wrapping span, which would reduce dom (and bundle size, and work from focus zone).

@dzearing I've spent a couple of hours trying to make the fix, but there were some not so simple styles that have to be made. Considering that the PR is to add focus on split button containers and some other components, I think that's outside of the scope of this specific issue. However, I have logged a bug for this issue at VSTS: 2209061 and will take a 2nd crack at it some other time.

chang47 · 2018-03-21T02:18:23Z

@dzearing I made the changes we discussed offline. Specifically here's what was added extra:

We now use the getFocusStyle() so we have the correct border effect on buttons now
Disabled button no longer have highlighting. In the case where only the primary button is disabled, we would put focus on the menu button and have the highlighting effect there
Regarding the bug we encountered where we had focus on a button and then open a menu on a different button and hit escape and the focus returns back to the first button. This should be expected, focus should always return back to whatever was previously focused, in this case, that should be the first button.

jspurlin · 2018-03-21T14:32:40Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx

      'aria-haspopup': true,
-      'aria-expanded': this._isExpanded
+      'aria-expanded': this._isExpanded,
+      'data-is-focusable': !isPrimaryButtonDisabled


Shouldn't this always not be focusable, even if the primary action is disabled? You want the interaction with a control to always be consistent.

I'm working with the assumption that if the primary button is disabled, we don't want to focus on the whole container and want to focus on the menu button. Would that not be the case?

No, that would not be the case. Focus should always only go on the container and the splitButton should always behave consistently for keyboard users

jspurlin · 2018-03-21T14:36:32Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx

    if (onMenuClick) {
      onMenuClick(ev, this);
    }



It seems strange that this calls onMenuClick (considering that this is keydown)... If this want to call onMenuClick, why isn't this calling _onMenuClick?

We want to trigger the primary action when we hit the enter button if we're focusing on the html element. This works for buttons, unfortunately, the container is a div so pressing enter won't trigger the onClick event.

I can move the code in onMenuClick and call it in both places, but they would both also reference onClick events. What do you think?

I think as long as _onToggleMenu is updating the focus correctly this is fine

This is resolved, we'll keep this code here and add the focus change to onToggleMenu

jspurlin · 2018-03-21T14:38:08Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx

      this.props.menuTriggerKeyCode !== null &&
-      ev.which === (this.props.menuTriggerKeyCode === undefined ? KeyCodes.down : this.props.menuTriggerKeyCode)) {
+      this._isValidMenuOpenKey(ev)) {
      this._onToggleMenu();


as part of _onToggleMenu could we set focus to the container before setting the state to toggle the menu? This would allow focus to return to the whole split button once the menu is dismissed

Would that be what we want to do? By doing this we would force the focus to go back to the menu in every scenario.

From my understanding, we want focus to go back to the previous focused element, wIth the suggested change, we always focus on the button when we close the menu.

The focus should always only be on the container (even if it was fired against the split button portion). We should never be setting focus on the individual portions of the splitButton

Added the changes here. Now we only don't focus the split button if everything is disabled.

jspurlin · 2018-03-21T14:42:19Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

    } as IContextualMenuItem;

    return React.createElement('button',
      assign({}, getNativeProps(itemProps, buttonProperties), {


Have splitButtons in menus been updated to behave the same way that splitButtons outside of menus behave? I think they will need to be updated at least to put focus on the container before the menu expands (so that focus will go back to the container when the menu collapses)

Style wise, it has the same hover affect, because I did use the getFocusStyle, unfortunately, I introduced a regression somewhere with the styles and it's not looking right. I'm looking back at how to fix it.

As for putting the focus back, you're right that's something we should have. I'll add that in.

The base button changes should apply to the contextual menu for free. the only possible change might be when we open a sub menu, but I don't think that'll be an issue as we have to focus on the split button in the contextual menu to open it.

In the end, after we close all the sub-menu we should put our focus back to the button that opened the contextual menu anyways.

I retract my earlier statement, like we discussed offline there was a case where if we keyboard on the menu and then hover over a split button menu and hit escape we would focus on the split button and not whole div. I've fixed that.

…ual menus

dzearing · 2018-03-20T19:19:23Z

packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx

+      buttonProps,
+      {
+        onClick: undefined,
+        tabIndex: -1,


Tabindex -1 still seems like a bad idea

REDMOND\chiechan added 6 commits March 8, 2018 14:17

made spin button and base button menu option un-focusable

2ffef62

added spin button test

979da51

added split button contextual menu change

e4ab4b4

got rid of un-needed code

3d95aca

added change files

64bc57b

added abillitiy to not focus on primary button for split button, but …

e08bf24

…only on the container

chang47 requested a review from joschect as a code owner March 9, 2018 17:58

joschect requested a review from dzearing March 9, 2018 21:18

REDMOND\chiechan added 5 commits March 9, 2018 14:34

enter on focus on split button trigger primary action

c16992b

added split button style for disabled

7bea2c0

prevent opening submenu if item is disabled

62cb458

tighten restrictions on opening split buttons to open with alt + down

efb7439

refactored button name and fixed test

f803332

chang47 closed this Mar 10, 2018

chang47 reopened this Mar 10, 2018

jspurlin reviewed Mar 12, 2018

View reviewed changes

jspurlin approved these changes Mar 12, 2018

View reviewed changes

REDMOND\chiechan and others added 2 commits March 13, 2018 10:36

updated bundle size

b2a4475

Merge branch 'master' into focus2

b1f2c39

REDMOND\chiechan added 3 commits March 13, 2018 11:51

changed on menu item click to bw able to take in a keyboard event

976c67f

Merge branch 'focus2' of https://github.com/chang47/office-ui-fabric-…

d3e1426

…react into focus2

updated package json

0aaefaa

dzearing reviewed Mar 13, 2018

View reviewed changes

fixed weird tabbing

bbc370f

jspurlin reviewed Mar 13, 2018

View reviewed changes

changed way we structured the focus

0fc2cc6

joschect reviewed Mar 13, 2018

View reviewed changes

joschect approved these changes Mar 13, 2018

View reviewed changes

added stop propagation

965a8e9

dzearing reviewed Mar 14, 2018

View reviewed changes

REDMOND\chiechan added 7 commits March 15, 2018 13:49

resolved merge conflicts

2603277

resolved merge conflict

aaa77c3

changed split button container key down to work with other buttons now

ad6e5df

update bundle size

e01436a

added correct split button container styles

a33679e

removed unnecessary attribute

93ba4ae

fixed focusing on menu button when primary button is disabled

d19e1a6

jspurlin reviewed Mar 21, 2018

View reviewed changes

REDMOND\chiechan added 7 commits March 21, 2018 13:47

added change to focus

24ae278

resolved merge conflict

0e7d887

added aria hidden

619711d

fixed span location and moved TODO

276fe96

added support to focus on whole container in split button for context…

d4436d4

…ual menus

added comment

843d758

fixed styling issues with buttons

9186034

dzearing approved these changes Mar 23, 2018

View reviewed changes

dzearing merged commit 0907222 into microsoft:master Mar 23, 2018

jspurlin mentioned this pull request Aug 15, 2018

"BaseButton: Add onFocusCapture to the split button container to focus the container instead of doing it in the menu onClick #5934

Merged

2 tasks

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019


		return <BaseButton { ...splitButtonProps } onMouseDown={ this._onMouseDown } />;

		return <BaseButton {...splitButtonProps} onMouseDown={ this._onMouseDown } tabIndex={ -1 } />;

Removing ability to focus with keyboarding on menu buttons for split buttons and and spin boxes #4222

Removing ability to focus with keyboarding on menu buttons for split buttons and and spin boxes #4222

Uh oh!

Conversation

chang47 commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

betrue-final-final commented Mar 9, 2018

Uh oh!

chang47 commented Mar 10, 2018

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

chang47 commented Mar 12, 2018

Uh oh!

jspurlin commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

chang47 commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joschect left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chang47 commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chang47 Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chang47 commented Mar 9, 2018 •

edited

Loading

jspurlin commented Mar 12, 2018 •

edited

Loading

jspurlin Mar 13, 2018 •

edited

Loading

chang47 commented Mar 21, 2018 •

edited

Loading

jspurlin Mar 21, 2018 •

edited

Loading

chang47 Mar 21, 2018 •

edited

Loading

jspurlin Mar 21, 2018 •

edited

Loading