Skip to content

RFC: 1st Rule of ARIA to converged components#17434

Merged
bsunderhus merged 3 commits intomasterfrom
bsunderhus/first-rule-of-aria-rfc
Jun 11, 2021
Merged

RFC: 1st Rule of ARIA to converged components#17434
bsunderhus merged 3 commits intomasterfrom
bsunderhus/first-rule-of-aria-rfc

Conversation

@bsunderhus
Copy link
Copy Markdown
Contributor

Reinforce First Rule of ARIA

This RFC proposes to promote using semantic elements as default and providing
options to opt-out of semantic elements through Shorthand props.

For the moment this RFC does not provide any form of API, is just a pattern to be followed. Perhaps in the future some internal API could be developed to better support this.

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Mar 16, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8e3b266:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@fabricteam
Copy link
Copy Markdown
Collaborator

fabricteam commented Mar 16, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 767 776 5000
BaseButton mount 883 875 5000
Breadcrumb mount 2536 2517 1000
ButtonNext mount 514 493 5000
Checkbox mount 1449 1491 5000
CheckboxBase mount 1239 1227 5000
ChoiceGroup mount 4509 4461 5000
ComboBox mount 940 947 1000
CommandBar mount 9694 9723 1000
ContextualMenu mount 6002 6049 1000
DefaultButton mount 1078 1084 5000
DetailsRow mount 3574 3605 5000
DetailsRowFast mount 3579 3646 5000
DetailsRowNoStyles mount 3396 3428 5000
Dialog mount 2073 2101 1000
DocumentCardTitle mount 138 135 1000
Dropdown mount 3124 3104 5000
FocusTrapZone mount 1748 1740 5000
FocusZone mount 1758 1714 5000
IconButton mount 1641 1690 5000
Label mount 324 324 5000
Layer mount 1738 1757 5000
Link mount 449 428 5000
MakeStyles mount 1729 1760 50000
MenuButton mount 1399 1397 5000
MessageBar mount 1929 1970 5000
Nav mount 3116 3096 1000
OverflowSet mount 984 990 5000
Panel mount 2062 1960 1000
Persona mount 808 794 1000
Pivot mount 1382 1368 1000
PrimaryButton mount 1218 1275 5000
Rating mount 7271 7331 5000
SearchBox mount 1248 1214 5000
Shimmer mount 2419 2410 5000
Slider mount 1862 1872 5000
SpinButton mount 4759 4909 5000
Spinner mount 436 393 5000
SplitButton mount 3082 3012 5000
Stack mount 475 472 5000
StackWithIntrinsicChildren mount 1512 1477 5000
StackWithTextChildren mount 4350 4431 5000
SwatchColorPicker mount 9797 9862 5000
Tabs mount 1367 1345 1000
TagPicker mount 2329 2296 5000
TeachingBubble mount 11436 11429 5000
Text mount 393 400 5000
TextField mount 1342 1350 5000
ThemeProvider mount 1138 1127 5000
ThemeProvider virtual-rerender 584 570 5000
ThemeProviderNext mount 6999 7104 5000
Toggle mount 785 792 5000
buttonNative mount 113 119 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
LabelMinimalPerf.default 392 351 1.12:1
TreeWith60ListItems.default 178 161 1.11:1
VideoMinimalPerf.default 628 566 1.11:1
PortalMinimalPerf.default 181 166 1.09:1
AlertMinimalPerf.default 263 244 1.08:1
AccordionMinimalPerf.default 157 147 1.07:1
FormMinimalPerf.default 388 361 1.07:1
ImageMinimalPerf.default 369 345 1.07:1
ListNestedPerf.default 527 498 1.06:1
RadioGroupMinimalPerf.default 440 417 1.06:1
SliderMinimalPerf.default 1577 1492 1.06:1
TextMinimalPerf.default 340 321 1.06:1
FlexMinimalPerf.default 273 260 1.05:1
CarouselMinimalPerf.default 448 429 1.04:1
ChatMinimalPerf.default 621 597 1.04:1
ChatWithPopoverPerf.default 353 339 1.04:1
DialogMinimalPerf.default 757 731 1.04:1
MenuMinimalPerf.default 837 801 1.04:1
StatusMinimalPerf.default 684 659 1.04:1
IconMinimalPerf.default 583 561 1.04:1
ChatDuplicateMessagesPerf.default 277 270 1.03:1
TableMinimalPerf.default 398 386 1.03:1
ToolbarMinimalPerf.default 904 875 1.03:1
TooltipMinimalPerf.default 956 929 1.03:1
ButtonMinimalPerf.default 167 163 1.02:1
ButtonSlotsPerf.default 513 504 1.02:1
CheckboxMinimalPerf.default 2654 2605 1.02:1
DividerMinimalPerf.default 367 360 1.02:1
EmbedMinimalPerf.default 4028 3965 1.02:1
HeaderMinimalPerf.default 349 343 1.02:1
InputMinimalPerf.default 1240 1221 1.02:1
CustomToolbarPrototype.default 3715 3629 1.02:1
ButtonOverridesMissPerf.default 1627 1606 1.01:1
CardMinimalPerf.default 533 529 1.01:1
DropdownManyItemsPerf.default 665 657 1.01:1
DropdownMinimalPerf.default 2975 2934 1.01:1
LayoutMinimalPerf.default 345 341 1.01:1
RefMinimalPerf.default 237 235 1.01:1
SplitButtonMinimalPerf.default 3619 3568 1.01:1
AttachmentSlotsPerf.default 1074 1073 1:1
DatepickerMinimalPerf.default 5245 5261 1:1
ItemLayoutMinimalPerf.default 1222 1226 1:1
ListCommonPerf.default 588 586 1:1
PopupMinimalPerf.default 561 562 1:1
ReactionMinimalPerf.default 361 360 1:1
SegmentMinimalPerf.default 334 333 1:1
TableManyItemsPerf.default 1840 1840 1:1
TreeMinimalPerf.default 753 750 1:1
HeaderSlotsPerf.default 731 738 0.99:1
ProviderMergeThemesPerf.default 1608 1617 0.99:1
SkeletonMinimalPerf.default 328 331 0.99:1
TextAreaMinimalPerf.default 464 471 0.99:1
MenuButtonMinimalPerf.default 1497 1522 0.98:1
ProviderMinimalPerf.default 950 965 0.98:1
ListMinimalPerf.default 482 495 0.97:1
AvatarMinimalPerf.default 181 188 0.96:1
BoxMinimalPerf.default 322 335 0.96:1
LoaderMinimalPerf.default 645 678 0.95:1
RosterPerf.default 1071 1129 0.95:1
AnimationMinimalPerf.default 384 408 0.94:1
ListWith60ListItems.default 608 644 0.94:1
AttachmentMinimalPerf.default 140 152 0.92:1
GridMinimalPerf.default 315 353 0.89:1

@size-auditor
Copy link
Copy Markdown

size-auditor bot commented Mar 16, 2021

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 4db9f8ef7210367634bef9a8e1a1658ee49a2618 (build)

Comment thread rfcs/convergence/first-rule-of-aria.md
Comment thread rfcs/convergence/first-rule-of-aria.md Outdated
Comment thread rfcs/convergence/first-rule-of-aria.md Outdated
Comment thread rfcs/convergence/first-rule-of-aria.md
Comment thread rfcs/convergence/first-rule-of-aria.md
Comment thread rfcs/convergence/first-rule-of-aria.md Outdated
Comment thread rfcs/convergence/first-rule-of-aria.md Outdated
@ling1726
Copy link
Copy Markdown
Contributor

We would still need to reeimplement any keyboard behaviour for elements like button or a right ? since we can use as prop for composition

@smhigley
Copy link
Copy Markdown
Contributor

We would still need to reeimplement any keyboard behaviour for elements like button or a right ? since we can use as prop for composition

I don't think this would be a good idea; if an author uses as, they would then be responsible for handling the implications of overriding the HTML element at the time they do so, rather than rewrite HTML logic within the base button/link component itself.

If someone is doing something like <Link as="div">, presumably they don't what the <a> functionality or semantics, and we probably shouldn't try to force it. For example, the example @bsunderhus originally had under "may sometimes not want to use semantic elements" was a button that had a link within it; i.e. something like this:

<Button as="div">
  <h3>Maybe structured content, even</h3>
  some text
  <a href="url">a link</a>
</Button>

If we add role="button" tabindex="0" + keyboard handlers to the top-level <Button> even when it's used as a div, that use case will fail, both because structurally buttons cannot contain headings and links, and sometimes maybe because the nested link (or other button) keyboard events will propagate up. Presumably the real-world use case would be something like a block-level button where the entire structured content is clickable, but it also has a specific secondary button within it. I generally think that if someone does as="div", or even as="button", they should get exactly what they asked for.

@ling1726
Copy link
Copy Markdown
Contributor

We would still need to reeimplement any keyboard behaviour for elements like button or a right ? since we can use as prop for composition

I don't think this would be a good idea; if an author uses as, they would then be responsible for handling the implications of overriding the HTML element at the time they do so, rather than rewrite HTML logic within the base button/link component itself.

If someone is doing something like <Link as="div">, presumably they don't what the <a> functionality or semantics, and we probably shouldn't try to force it. For example, the example @bsunderhus originally had under "may sometimes not want to use semantic elements" was a button that had a link within it; i.e. something like this:

<Button as="div">
  <h3>Maybe structured content, even</h3>
  some text
  <a href="url">a link</a>
</Button>

If we add role="button" tabindex="0" + keyboard handlers to the top-level <Button> even when it's used as a div, that use case will fail, both because structurally buttons cannot contain headings and links, and sometimes maybe because the nested link (or other button) keyboard events will propagate up. Presumably the real-world use case would be something like a block-level button where the entire structured content is clickable, but it also has a specific secondary button within it. I generally think that if someone does as="div", or even as="button", they should get exactly what they asked for.

I personally agree with this approach, but historically (and currently) this has not been the case for Fluent, so we should ideally come to an agreement as a team about this.

@smhigley
Copy link
Copy Markdown
Contributor

@ling1726 makes sense, I think it'd be a good change for converged components as long as we have the right docs n warnings :)

@bsunderhus
Copy link
Copy Markdown
Contributor Author

bsunderhus commented Mar 22, 2021

We would still need to reeimplement any keyboard behaviour for elements like button or a right ? since we can use as prop for composition

I got to say that I'm not sure about that. The point of needing to reeimplement any keyboard/attributes is valid IMO to ensure A11y, but I also understand that there're clear use cases that if we do that than things will go south, like on @smhigley example. I'm getting this feeling that perhaps the as property is not enough to address this issue.

Perhaps we could have something else to declare that even though we want to opt-out of the semantic element, we still want it's functionality...

@bsunderhus bsunderhus requested review from Hotell and jurokapsiar and removed request for kubkon May 19, 2021 16:12

---

If isn't equivalent to native elements but can benefit from using native elements in it's implementation - `Accordion`, `Carousel`, `Disclosure`, `MenuButton`, `Menu`, `MenuBar`, `Tabs`
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.

Can you make it clearer what these benefits are ?

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.

I would prefer for this to be role oriented, because if the role differs from the semantic HTML element it might not be very useful if you would need to reimplement the behaviour of those native elements.

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.

Some of the benefits of using a <button> for tabs, accordion triggers, menu buttons, etc. include high contrast styles by default and keyboard invocation by default. There may be other benefits to using a natively interactive element that I'm not aware of, e.g. in assistive tech we don't test for.

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.

But I do agree there are some limits (e.g. <button> should only be used for controls that perform an action on click), and in the case of components like Menu, the benefit is likely less because we're already managing keyboard interaction, and button HCM colors aren't as important for menuitems as for standalone buttons.

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.

I didn't think about HCM colors, that's a good call out, let's add the benefits (i.e. interaction + HCM colors) in this RFC


If isn't equivalent to native elements but can benefit from using native elements in it's implementation - `Accordion`, `Carousel`, `Disclosure`, `MenuButton`, `Menu`, `MenuBar`, `Tabs`

- Should treat internal elements that could follow previous category as such (e.g: `Accordion` has an internal element with role `button`, this element should previous category)
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.

I would say that you should just move accordion to the previous category, in this case there is a native element for that role

Copy link
Copy Markdown
Contributor

@smhigley smhigley May 24, 2021

Choose a reason for hiding this comment

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

do you mean details/summary? They're definitely an interesting pair, I'm actually not sure those specific elements have much (or any) accessibility benefits 😅.

Maybe it'd be worth having a section on exceptions to the "native elements > aria" rule for things like details/summary, , the title attribute, and the dialog element? 😅

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.

Ah sorry wasn't clear enough, my point was that Accordion design pattern actually does explicitly call for the use of a button element, so it doesn't exactly fit the bill of 'can use native element' rather 'should use native element'

Copy link
Copy Markdown
Contributor

@ling1726 ling1726 left a comment

Choose a reason for hiding this comment

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

I agree that we should use native elements when the role being used actually has an exacty semantic HTML equivalent, either in base components or more complicated ones.

Copy link
Copy Markdown
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

I agree with the proposal in current state and especially I like that it's highlighted that some components should not follow this rule 👍


- Styles override to remove styles from semantic elements will be necessary
- In cases of heading, a lvl attribute might be required
- In cases of virtualization, semantic elements are not recommended (big problem for lists, menus, tables, and other components that depend on a group of items)
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.

might be worth specifying this is because of performance costs of layout recalc, especially for table (does anyone know if this is also true for lists?)

Copy link
Copy Markdown
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

Agree with the proposal. Let's see how this stands when we implement a Dropdown.

<!-- Style only -->
<div class="button-class">
This is a div that looks like Button but doesn't behave as such therefore, I can add a
<a href="/somewhere">Link inside of it</a>
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.

why not use the example of <a href=... class=button-class> here? I'd that that be a more accurate example?

@bsunderhus bsunderhus force-pushed the bsunderhus/first-rule-of-aria-rfc branch from d7b54c0 to f3caf57 Compare June 11, 2021 11:00
@bsunderhus bsunderhus merged commit 42fa7ee into master Jun 11, 2021
@bsunderhus bsunderhus deleted the bsunderhus/first-rule-of-aria-rfc branch June 11, 2021 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: RFC Request for Feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants