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 7 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": "rebeba@microsoft.com"
}
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>);

@joschect joschect May 4, 2018

Copy link
Copy Markdown
Contributor

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?

@jspurlin jspurlin May 10, 2018

Copy link
Copy Markdown
Contributor

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)

@rebeccaballantyne rebeccaballantyne May 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

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.

@jspurlin jspurlin May 10, 2018

Copy link
Copy Markdown
Contributor

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;

@joschect joschect May 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@rebeccaballantyne rebeccaballantyne May 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@ThomasMichon ThomasMichon May 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's worth the breaking change, because this makes host apps aware of the issue.

}

export interface IBasePickerSuggestionsProps {
Expand Down
11 changes: 6 additions & 5 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,14 @@ export class Suggestions<T> extends BaseComponent<ISuggestionsProps<T>, ISuggest
{ footerTitle(this.props) }
</div>) : (null)
}
{ (!isLoading && !isSearching && suggestions && suggestions.length > 0 && suggestionsAvailableAlertText) ?
(<span
role='alert'
{
(<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