Skip to content

Conversation

@paterasMSFT
Copy link
Contributor

Pull request checklist

Description of changes

Added a property to ComboBox to specify the value for its button's data-is-focusable attribute. This is useful when the ComboBox is part of a FocusZone, as setting the value to false allows the button to be skipped with keyboard navigation.

I defaulted it to true, to avoid making a silent, subtle change for existing users. However, I believe the most frequent use case would be to set it to false, as that button doesn't make much sense in a keyboard navigation context. Any advice on what the appropriate default setting should be would be appreciated.

@dzearing dzearing changed the title Combo box button data is focusable ComboBox button data is focusable Feb 22, 2018
@jspurlin
Copy link
Contributor

Yep. this holds true for spitButtons and spinButtons as well

@jspurlin jspurlin changed the title ComboBox button data is focusable ComboBox/SpinButton/SplitButton individual buttons should have data-is-focusable="false" Feb 22, 2018
@jspurlin
Copy link
Contributor

@paterasMSFT The button should always be data-is-focusable={false}... it should never be focusable

@jspurlin jspurlin changed the title ComboBox/SpinButton/SplitButton individual buttons should have data-is-focusable="false" ComboBox button should have data-is-focusable="false" Feb 22, 2018
@paterasMSFT
Copy link
Contributor Author

It occurs to me that if the button is no longer keyboard navigatable, should we also remove aria-hidden=true and tabindex=-1? Do those still serve a purpose?

I know tabindex=-1 means it's not keyboard navigatable (something I surmise the FocusZone works around), but is still focusable (presumably by mouse?).

@jspurlin jspurlin merged commit f803791 into microsoft:master Feb 23, 2018
@jspurlin
Copy link
Contributor

just got your comment. Yes, the tabindex can be removed, but we should hold off on removing the aria-hidden, there's still some unresolved issues around how the comboBox gets build for accessibility where it currently doesn't correctly represent the control correctly if the button is visible

@paterasMSFT
Copy link
Contributor Author

Now that the PR is closed, how should I make that change?

@jspurlin
Copy link
Contributor

No, we can make that update when we deal with the unresolved issues that exist on the browser/screen reader side of things

Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Feb 27, 2018
* master: (28 commits)
  Scrollable pane sort stickies (microsoft#4111)
  Allow ScrollablePane to accept native properties. (microsoft#4095)
  Sticky (microsoft#4091)
  Applying package updates.
  [ColorRectangle, Sticky] Fixed null root refs (microsoft#4099)
  DatePicker: order of callbacks for onSelectDate and onAfterMenuDismiss (microsoft#4092)
  Applying package updates.
  Alhenry fix split button props (microsoft#4090)
  Fixing ComboBox styling by reverting button classname move (microsoft#4088)
  Update CODEOWNERS
  Undoing terrible change.
  [DetailsList] Fixed focus test (microsoft#4087)
  Added icons package screener test (microsoft#4082)
  ContextualMenu: Fix ContextualMenuUtility imports (microsoft#4085)
  [ContextualMenu] Made disabled buttons focusable (microsoft#4074)
  Convert Check to mergeStyles (microsoft#3880)
  [DetailsList] Add public focusIndex function (microsoft#3852)
  ComboBox button should have data-is-focusable="false" (microsoft#4070)
  Applying package updates.
  Focus Zone: Allow Tab to Skip Selection (microsoft#4061)
  ...
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 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.

ComboBox: Allow users to set the button's data-is-focusable value

3 participants