Fixes duplicate reading of suggestions on people picker #4765
Conversation
rebeccaballantyne
changed the title
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "Puts additional alert on selected suggestions behind a prop", | ||
| "type": "patch" |
There was a problem hiding this comment.
This should be minor given the new prop.
| if (this.props.enableSelectedSuggestionAlert) { | ||
| const selectedSuggestion = currentIndex > -1 ? this.suggestionStore.getSuggestionAtIndex(this.suggestionStore.currentIndex) : undefined; | ||
| const selectedSuggestionAlertText = selectedSuggestion ? selectedSuggestion.ariaLabel : undefined; | ||
| selectedSuggestionAlert = (<div className={ styles.screenReaderOnly } role='alert' id='selected-suggestion-alert' aria-live='assertive'>{ selectedSuggestionAlertText } </div>); |
| * enables proper screen reader behavior for each suggestion. It should not be set for modern browsers (Edge, Chrome). | ||
| * @default false | ||
| */ | ||
| enableSelectedSuggestionAlert?: boolean; |
There was a problem hiding this comment.
Doesn't this essentially make it a breaking change for IE 11? Since by default it's false, anything that's consuming it won't know to have a check for IE11 and won't pass through the variable, so screen readers won't read it properly. @dzearing and @cliffkoh should the picker check for ie 11 itself maybe?
There was a problem hiding this comment.
This does break IE 11, but it fixes Chrome and Edge. Do we have browser detection logic in fabric? Would love to not even have a new prop needed here.
There was a problem hiding this comment.
I think it's worth the breaking change, because this makes host apps aware of the issue.
…n available so it reads
rebeccaballantyne
force-pushed
the
rebeba/suggestions-alert
branch
from
581b253 to
18e8c63
Compare
| if (this.props.enableSelectedSuggestionAlert) { | ||
| const selectedSuggestion = currentIndex > -1 ? this.suggestionStore.getSuggestionAtIndex(this.suggestionStore.currentIndex) : undefined; | ||
| const selectedSuggestionAlertText = selectedSuggestion ? selectedSuggestion.ariaLabel : undefined; | ||
| selectedSuggestionAlert = (<div className={ styles.screenReaderOnly } role='alert' id='selected-suggestion-alert' aria-live='assertive'>{ selectedSuggestionAlertText } </div>); |
There was a problem hiding this comment.
Why is this element assertive while the one below is polite, shouldn't they be the same? (Polite seems like the correct level to use here)
There was a problem hiding this comment.
For IE. The only reason this alert exists at all is for IE, and in traditional IE fashion, it does not read unless it is set to "assertive".
Definitely agree that polite seems like the correct level here, but since this alert was created so it would work in IE, we need it to be assertive.
There was a problem hiding this comment.
Okay, in that case should the live region below also use assertive?
…i-fabric-react into parallel-tsc * 'parallel-tsc' of https://github.com/Markionium/office-ui-fabric-react: Variants: have project use OUFR instead of just styling (microsoft#4854) Add more customization hooks to ProgressIndicator (microsoft#4566) Issue#4832: Break BaseButton Types hard dependency from class ContextualMenu (microsoft#4845) Applying package updates. Revert react portals change (microsoft#4840) Update ImageOverview.md Fixes duplicate reading of suggestions on people picker (microsoft#4765) Persona: Deprecate primaryText (microsoft#4811) Experiments: Fix Fluent theme color names (microsoft#4834) Applying package updates. Add JasonGore to command bar codeowners Fix index import (microsoft#4826) Added overflowMenuProps property to CommandBar (microsoft#4818) Fluent theme: Fix imports to use relative paths (microsoft#4831) ContextualMenuItem: adding secondaryText (microsoft#4788) ComboBox: Option Performance Optimization (microsoft#4782)
* master: (95 commits) Variants: have project use OUFR instead of just styling (microsoft#4854) Add more customization hooks to ProgressIndicator (microsoft#4566) Issue#4832: Break BaseButton Types hard dependency from class ContextualMenu (microsoft#4845) Applying package updates. Revert react portals change (microsoft#4840) Update ImageOverview.md Fixes duplicate reading of suggestions on people picker (microsoft#4765) Persona: Deprecate primaryText (microsoft#4811) Experiments: Fix Fluent theme color names (microsoft#4834) Applying package updates. Add JasonGore to command bar codeowners Fix index import (microsoft#4826) Added overflowMenuProps property to CommandBar (microsoft#4818) Fluent theme: Fix imports to use relative paths (microsoft#4831) ContextualMenuItem: adding secondaryText (microsoft#4788) ComboBox: Option Performance Optimization (microsoft#4782) Marqueeselection style update (microsoft#4803) Applying package updates. FocusZone: Add the ability to stop focus from propagating outside the FocusZone (microsoft#4823) Unknown persona coin (microsoft#4809) ...
6 participants
Pull request checklist
$ npm run changeDescription of changes
Puts the additional alert that was added in this PR (#2944) behind a prop. This alert was originally added to enable Narrator to read the selected suggestion in IE11 - however, the alert squashes suggestionsAvailableAlertText in other browsers and also results in duplicate reading of each selected suggestion.
Since Fabric doesn't have browser detection, putting it behind a prop allows consumers of the PeoplePicker component to specify when the additional alert should be read based off of the browser.
Focus areas to test
Toggle T/F of the prop in PeoplePicker.Types.Example.tsx for "NormalPeoplePicker" and try with Narrator on in Edge, Chrome, and IE. When the prop is set to true, you will hear each suggestion announced in IE on keyboarding, and multiple readings of each suggestion in Chrome/ Edge. When the prop is set to false, you will hear each suggestion announced once in Chrome/ Edge and the suggestion not announced at all in IE.