Skip to content

Fixes duplicate reading of suggestions on people picker #4765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joschect merged 8 commits into from
May 10, 2018
Merged

Fixes duplicate reading of suggestions on people picker #4765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2797c1d
Fixes for suggestions being read twice
rebeccaballantyne01 May 3, 2018
451279a
Rush change file
rebeccaballantyne01 May 3, 2018
d88c280
Changes to minor
rebeccaballantyne01 May 7, 2018
083aacf
Updating snapshot
rebeccaballantyne01 May 8, 2018
11d1f3c
Updating comment to reflect aria-activedescendant
rebeccaballantyne01 May 10, 2018
44d046a
Making suggestions available live region, dynamically adding text whe…
rebeccaballantyne01 May 10, 2018
18e8c63
Updating snapshot again
rebeccaballantyne01 May 10, 2018
06d9391
Fixing formatting issues
rebeccaballantyne01 May 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/rebeba-suggestions-alert_2018-05-03-22-40.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Puts additional alert on selected suggestions behind a prop",
"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "[email protected]"
}
11 changes: 8 additions & 3 deletions packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,15 @@ export class BasePicker<T, P extends IBasePickerProps<T>> extends BaseComponent<
} = this.props;

const currentIndex = this.suggestionStore.currentIndex;
const selectedSuggestion = currentIndex > -1 ? this.suggestionStore.getSuggestionAtIndex(this.suggestionStore.currentIndex) : undefined;
const selectedSuggestionAlert = selectedSuggestion ? selectedSuggestion.ariaLabel : undefined;
const activeDescendant = currentIndex > -1 ? 'sug-' + currentIndex : undefined;

let selectedSuggestionAlert = undefined;
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>);
Copy link
Contributor

@joschect joschect May 4, 2018

Choose a reason for hiding this comment

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

More of just a general point, we should really have a screen reader component or something that does this and handles all those attributes so we can have them standardized. @cliffkoh or @dzearing what do you think about that?

Copy link
Contributor

@jspurlin jspurlin May 10, 2018

Choose a reason for hiding this comment

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

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)

Copy link
Author

@rebeccaballantyne rebeccaballantyne May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jspurlin jspurlin May 10, 2018

Choose a reason for hiding this comment

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

Okay, in that case should the live region below also use assertive?

}

return (
<div
ref={ this.root }
Expand All @@ -192,7 +197,7 @@ export class BasePicker<T, P extends IBasePickerProps<T>> extends BaseComponent<
direction={ FocusZoneDirection.bidirectional }
isInnerZoneKeystroke={ this._isFocusZoneInnerKeystroke }
>
<div className={ styles.screenReaderOnly } role='alert' id='selected-suggestion-alert' aria-live='assertive'>{ selectedSuggestionAlert } </div>
{ selectedSuggestionAlert }
<SelectionZone selection={ this.selection } selectionMode={ SelectionMode.multiple }>
<div className={ css('ms-BasePicker-text', styles.pickerText, this.state.isFocused && styles.inputFocused) } role={ 'list' }>
{ this.renderItems() }
Expand Down
7 changes: 7 additions & 0 deletions packages/office-ui-fabric-react/src/components/pickers/BasePicker.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ export interface IBasePickerProps<T> extends React.Props<any> {
* A callback to override the default behavior of adding the selected suggestion on dismiss.
*/
onDismiss?: (ev?: any, selectedItem?: T) => void;
/**
* Adds an additional alert for the currently selected suggestion. This prop should be set to true for IE11 and below, as it
* enables proper screen reader behavior for each suggestion (since aria-activedescendant does not work with IE11).
* It should not be set for modern browsers (Edge, Chrome).
* @default false
*/
enableSelectedSuggestionAlert?: boolean;
}

export interface IBasePickerSuggestionsProps {
Expand Down
9 changes: 6 additions & 3 deletions packages/office-ui-fabric-react/src/components/pickers/Suggestions/Suggestions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,13 +193,16 @@ export class Suggestions<T> extends BaseComponent<ISuggestionsProps<T>, ISuggest
{ footerTitle(this.props) }
</div>) : (null)
}
{ (!isLoading && !isSearching && suggestions && suggestions.length > 0 && suggestionsAvailableAlertText) ?
{
(<span
role='alert'
aria-live='polite'
className={ css('ms-Suggestions-suggestionsAvailable', styles.suggestionsAvailable) }
>
{ suggestionsAvailableAlertText }
</span>) : (null)
{ (!isLoading && !isSearching && suggestions && suggestions.length > 0 && suggestionsAvailableAlertText) ?
suggestionsAvailableAlertText : null
}
</span>)
}
</div>
);
Expand Down
10 changes: 10 additions & 0 deletions ...i-fabric-react/src/components/pickers/Suggestions/__snapshots__/Suggestions.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1601,6 +1601,11 @@ exports[`Suggestions renders a list properly 1`] = `
</div>
</div>
</div>
<span
aria-live="polite"
className="ms-Suggestions-suggestionsAvailable"
role="alert"
/>
</div>
`;

Expand Down Expand Up @@ -3205,5 +3210,10 @@ exports[`Suggestions scrolls to selected index properly 1`] = `
</div>
</div>
</div>
<span
aria-live="polite"
className="ms-Suggestions-suggestionsAvailable"
role="alert"
/>
</div>
`;
16 changes: 0 additions & 16 deletions ...ages/office-ui-fabric-react/src/components/pickers/__snapshots__/BasePicker.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,6 @@ exports[`Pickers BasePicker renders BasePicker correctly 1`] = `
onMouseDownCapture={[Function]}
role="presentation"
>
<div
aria-live="assertive"
className={undefined}
id="selected-suggestion-alert"
role="alert"
>

</div>
<div
className="ms-SelectionZone"
onClick={[Function]}
Expand Down Expand Up @@ -82,14 +74,6 @@ exports[`Pickers TagPicker renders TagPicker correctly 1`] = `
onMouseDownCapture={[Function]}
role="presentation"
>
<div
aria-live="assertive"
className={undefined}
id="selected-suggestion-alert"
role="alert"
>

</div>
<div
className="ms-SelectionZone"
onClick={[Function]}
Expand Down