Skip to content
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.

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

Add role to FocusZone in BasePickerListBelow #10867

Merged
merged 2 commits into from
Oct 28, 2019

Conversation

mattlind
Copy link
Collaborator

@mattlind mattlind commented Oct 16, 2019

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

Added role attribute to the FocusZone that contains the selected items for BasePickerListBelow. Previously there was no role set so adding a role like "list-item" to an item produced by the onRenderItem callback didn't read out in screen readers as item number n of m.

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

@size-auditor
Copy link

size-auditor bot commented Oct 16, 2019

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react Pickers 263.005 kB 263.017 kB ExceedsBaseline     12 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 58691b0adf66ee810bbc9b055c92b76706c67dc0 (build)

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Oct 16, 2019

Component Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 785 807
BaseButton (experiments) 1060 1055
DefaultButton 1055 1060
DefaultButton (experiments) 2041 2020
DetailsRow 3343 3444
DetailsRow (fast icons) 3404 3491
DetailsRow without styles 3161 3214
DocumentCardTitle with truncation 33974 33964
MenuButton 1379 1391
MenuButton (experiments) 3755 3656
PrimaryButton 1284 1259
PrimaryButton (experiments) 2130 2103
SplitButton 2931 2927
SplitButton (experiments) 7225 7250
Stack 522 504
Stack with Intrinsic children 1215 1198
Stack with Text children 4557 4505
Text 410 412
Toggle 915 902
Toggle (experiments) 2369 2418
button 63 69

@ecraig12345 ecraig12345 changed the title Base picker list below role Add role to FocusZone in BasePickerListBelow Oct 16, 2019
@joschect
Copy link
Contributor

@mattlind Thanks for submitting this fix! It looks like it would break aria rules for users who don't have role="listitem" already on their selected items. I think that either adding selectedItemsContainerProps: IFocusZoneProps' or renderSelectedItemsContainer` to props might be another way of doing this.

@mattlind
Copy link
Collaborator Author

mattlind commented Oct 21, 2019

@mattlind Thanks for submitting this fix! It looks like it would break aria rules for users who don't have role="listitem" already on their selected items. I think that either adding selectedItemsContainerProps: IFocusZoneProps' or renderSelectedItemsContainer` to props might be another way of doing this.

I did it this way because that was how the other instance was doing the same thing above.

<SelectionZone selection={this.selection} selectionMode={SelectionMode.multiple}>
            <div className={classNames.text}>
              {items.length > 0 && (
                <span id={this._ariaMap.selectedItems} className={classNames.itemsWrapper} role={'list'}>
                  {this.renderItems()}
                </span>
              )}

@joschect
Copy link
Contributor

@mattlind That makes sense then. Approved to bring everything in line.

@mattlind mattlind closed this Oct 28, 2019
@mattlind mattlind reopened this Oct 28, 2019
@joschect joschect merged commit d78a5c1 into microsoft:master Oct 28, 2019
@msft-github-bot
Copy link
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@shivasai09
Copy link
Contributor

is it possible to cherry pick this change to version 6?

chakrisatish84 pushed a commit to chakrisatish84/office-ui-fabric-react that referenced this pull request Dec 5, 2019
msft-github-bot pushed a commit that referenced this pull request Dec 10, 2019
#### Pull request checklist

- [ ] Addresses an existing issue: Fixes #0000
- [ ] Include a change request file using `$ yarn change`

#### Description of changes

We are having one accessibility work item (Bug 3797473) and to fix this we need to add a change in Fabric.
 
But we noticed some one has already added a fix for this and released that In 7.55.3 (PR #10867). Since it may take some time to migrate to Fabric 7 this PR is cherrypicking/backporting this change to Fabric 6.


#### Focus areas to test

(optional)


###### Microsoft Reviewers: [Open in CodeFlow](http://wpcp.azurewebsites.net/CodeFlowProtocolProxy2.php?pullrequest=https://github.com/OfficeDev/office-ui-fabric-react/pull/11387)
@msft-github-bot
Copy link
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants