Skip to content

ContextualMenu has aria-labelledBy referencing element not in DOM#4963

Merged
jspurlin merged 5 commits intomicrosoft:masterfrom
rpsenski:BaseButton_ContextMenu_AriaLabelledBy
May 25, 2018
Merged

ContextualMenu has aria-labelledBy referencing element not in DOM#4963
jspurlin merged 5 commits intomicrosoft:masterfrom
rpsenski:BaseButton_ContextMenu_AriaLabelledBy

Conversation

@rpsenski
Copy link
Copy Markdown
Contributor

@rpsenski rpsenski commented May 23, 2018 •
edited by fabricteam
Loading

Pull request checklist

Description of changes

  1. Factored out logic used to determine if there's text on the button that will launch the context menu
  2. Used that logic to control whether an element with the text is created AND used it to determine if we need aria-labelledBy on the ContextMenu itself (achieved through labelElementId property in menuProps)
  3. Also check that ariaLabel AND that labelElementId aren't explicitly set on menuProps before choosing to inherit from the button text

Focus areas to test

Easily tested using OverflowSet:

  1. Given when there is no text on the button and the button is clicked, then there should be no aria-labelledBy on the ContextualMenu produced by clicking the button
  2. Given when _onRenderOverflowButton callback prop sets ariaLabel on the menuProps it provides and the button does or doesn't have text, then the ContextualMenu should have an aria-label but no aria-labelledBy.
  3. Given when _onRenderOverflowButton callback prop sets labelElementId on the menuProps to the id of some control A, and the button does or doesn't have text, then the ContextualMenu should have an aria-labelledBy equal to the id of control A, not the id of the text element on the button (if text was provided)

Reviewers:

@joschect
@jspurlin

Microsoft Reviewers: Open in CodeFlow

rpsenski added 3 commits May 23, 2018 09:28
@rpsenski
Bug: ContextualMenu has aria-labelledBy pointing to non-existing element
195a4ab 
Fix:
- Factor out logic to test whether button has associated text
- Check menuProps for explicit label/labelledby
- Do not add labelledBy unless there's no explicit label and text exists
@rpsenski
Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…
b2fb996 
…c-react into BaseButton_ContextMenu_AriaLabelledBy
@rpsenski
change description
fafd1c4
@JasonGore
Copy link
Copy Markdown
Member

JasonGore commented May 23, 2018 •
edited
Loading

Hmm, this seems like a good candidate for a test with and without text to verify prop does and doesn't appear as expected (and to protect against regression)

jspurlin
jspurlin approved these changes May 23, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jspurlin jspurlin left a comment

Choose a reason for hiding this comment

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

Let's add a test to help make sure this doesn't regress, but other than that this looks good!

rpsenski
rpsenski commented May 24, 2018
View reviewed changes
packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx Outdated
} = this.props;

if (text || typeof (children) === 'string' || secondaryText) {
if (this._hasText() || secondaryText) {
Copy link
Copy Markdown
Contributor Author

@rpsenski rpsenski May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Wanted to point out that the refactor actually made this a stricter check. Before, if text was null and typeof (children) === 'string' evaluated to true, the if was true. Now, text must be undefined for typeof (children) check to make the if truthy. I think this was the original intention because falling back to string children for text is supposed to be for backward compatibility and assumes text property is not present, but it's still a change that I want scrutinized by reviewers. #Resolved

Copy link
Copy Markdown
Contributor

@jspurlin jspurlin May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for bring this up. While I agree that this seems safe, let's not change the logic because it may have a ripple effect for some consumers. Feel free to log a bug and list it in a comment inside of _hasText so that it can be addressed later

Copy link
Copy Markdown
Contributor Author

@rpsenski rpsenski May 24, 2018

Choose a reason for hiding this comment

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

Reverted to reduce risk

In reply to: 190656224 [](ancestors = 190656224)

Copy link
Copy Markdown
Contributor Author

@rpsenski rpsenski May 24, 2018

Choose a reason for hiding this comment

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

reverted in Iteration 5

In reply to: 190709346 [](ancestors = 190709346)

@rpsenski
Copy link
Copy Markdown
Contributor Author

rpsenski commented May 24, 2018

Iteration 4

In reply to: 391440443 [](ancestors = 391440443)

@jspurlin jspurlin merged commit 8447135 into microsoft:master May 25, 2018
@rpsenski rpsenski deleted the BaseButton_ContextMenu_AriaLabelledBy branch May 25, 2018 13:20
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request May 29, 2018
@Markionium
Merge branch 'master' into parallel-tsc
aea1b4f 
* master:
  Applying package updates.
  Shimmer: refactor in preparation for migration to OUFR (microsoft#4958)
  Applying package updates.
  Theming: fix error colors (microsoft#4969)
  MaskedTextField: Added event callback passthrough (microsoft#4956)
  Experiments/Nav component: Enable auto expand until the next manual expand disables the auto expand (microsoft#4996)
  Applying package updates.
  Experiments/Nav component: Auto select/expand based on the selectedKey prop (microsoft#4984)
  StickyPane: fix Array.from in Ie (microsoft#4982)
  ContextualMenu has aria-labelledBy referencing element not in DOM (microsoft#4963)
  Keyboard support for the slim version of experiments/Nav component and added aria attributes (microsoft#4981)
  Move ghdocs to wiki (microsoft#4977)
  Remove build artifacts (microsoft#4976)
  Revisited the Multi-select Combo box initial state selection fix (microsoft#4884)
  Applying package updates.
  readme cleanup (microsoft#4972)
  Theming: add the bodyBackgroundDarker semantic slot (microsoft#4957)
  Improvements for auto-select opt-in mode (microsoft#4914)
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request May 29, 2018
@Markionium
Merge branch 'master' into mapol/allow-overriding-root-element-on-link
6129f2a 
* master: (85 commits)
  Applying package updates.
  Shimmer: refactor in preparation for migration to OUFR (microsoft#4958)
  Applying package updates.
  Theming: fix error colors (microsoft#4969)
  MaskedTextField: Added event callback passthrough (microsoft#4956)
  Experiments/Nav component: Enable auto expand until the next manual expand disables the auto expand (microsoft#4996)
  Applying package updates.
  Experiments/Nav component: Auto select/expand based on the selectedKey prop (microsoft#4984)
  StickyPane: fix Array.from in Ie (microsoft#4982)
  ContextualMenu has aria-labelledBy referencing element not in DOM (microsoft#4963)
  Keyboard support for the slim version of experiments/Nav component and added aria attributes (microsoft#4981)
  Move ghdocs to wiki (microsoft#4977)
  Remove build artifacts (microsoft#4976)
  Revisited the Multi-select Combo box initial state selection fix (microsoft#4884)
  Applying package updates.
  readme cleanup (microsoft#4972)
  Theming: add the bodyBackgroundDarker semantic slot (microsoft#4957)
  Improvements for auto-select opt-in mode (microsoft#4914)
  Applying package updates.
  Revert ChoiceGroup change in 5.0 to minimize potential partner impact. (microsoft#4962)
  ...
@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.

Reviewers

@jspurlin jspurlin jspurlin approved these changes

@dzearing dzearing Awaiting requested review from dzearing

@JasonGore JasonGore Awaiting requested review from JasonGore

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Invalid aria-labelledby for contexualmenu container

3 participants

@rpsenski @JasonGore @jspurlin