feat(accessibility): add behaviours for tabs, tablists and toolbars #83

silviuaavram · 2018-08-13T10:59:43Z

This PR adds accessibility behaviours for Menus styled as a Tablist or Toolbar, along with examples of use.
As a side task, I've added the possibility to handle props in the component from the accessibility behaviour. I did it to be able to add attributes like 'aria-label' to other parts of the component other than the default. For instance, in the Menu case, I want to add the 'aria-label' passed as a prop to the component into the <a> element instead of the <li>.

sophieH29 · 2018-08-13T16:18:39Z

src/lib/accessibility/Behaviors/Tab/TabBehavior.ts

+    },
+    anchor: {
+      role: 'tab',
+      'aria-selected': !!props['active'],


I think you won't need double exclamation mark here, it can be just set to props['active'] in string

please test this with screen readers - double exclamation marks will turn anything into true/false. What is the behavior described by ARIA best practices? Should the screen reader always read "not selected" when you navigate to a tab that is not selected? or should it just read "selected" on the tab that is selected? if latter is the preferred way, I think it should be props['active'] ? true : undefined

It was tested with both double exclamation and without. It works in both cases, reason why I added is because of ambiguity. there is also a double exclamation for SelectableListItemBehavior, 'aria-selected': !!props['active'], . How should we go consistent?

Aria states we should have true and false as values. VoiceOver screen reader will not read 'not selected' if we have "false", it will just ignore it. ChromeVox, however, will read it as 'not selected'. Afaik we support VoiceOver only, but even with ChromeVox behaviour should be ok, it's just a different way to read it, and Aria accepts it too. https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html

sophieH29 · 2018-08-13T16:19:10Z

src/lib/accessibility/Behaviors/Toolbar/ToolbarBehavior.ts

+    root: {
+      role: 'toolbar',
+      'aria-label': props['aria-label'],
+      'aria-disabled': !!props['disabled'],


same here regarding exclamation marks

similar to tab behavior. wouldn't the screen reader with this read 'not disabled' each time user navigates to a button? note that different readers might read it differently.

will remove the double excl marks if we decide to be consistent with that approach.

jurokapsiar · 2018-08-14T07:57:26Z

src/lib/accessibility/Behaviors/Tab/TabListBehavior.ts

+  attributes: {
+    root: {
+      role: 'tablist',
+      'aria-label': props['aria-label'],


I think this is not needed. It will come out of the box through the rest/unhandledProps.

will remove it, thanks.

actually, this behavior (where rest or unhandledProps are passed down the components' tree) is not guaranteed by consumer of accessibility module. If we would like accesibility module being an independent unit, these lines should be here

jurokapsiar · 2018-08-14T07:57:53Z

src/lib/accessibility/Behaviors/Toolbar/ToolbarBehavior.ts

+  attributes: {
+    root: {
+      role: 'toolbar',
+      'aria-label': props['aria-label'],


I think this is not needed. It will come out of the box through the rest/unhandledProps.

this one too

jurokapsiar · 2018-08-14T08:01:32Z

src/lib/accessibility/index.ts

@@ -12,3 +12,6 @@ export { default as InputBehavior } from './Behaviors/Input/InputBehavior'
 export { default as TreeBehavior } from './Behaviors/Tree/TreeBehavior'
 export { default as TreeItemBehavior } from './Behaviors/Tree/TreeItemBehavior'
 export { default as GroupBehavior } from './Behaviors/Tree/GroupBehavior'
+export { default as TabBehavior } from './Behaviors/Tab/TabBehavior'
+export { default as TabListBehavior } from './Behaviors/Tab/TabListBehavior'
+export { default as ToolbarBehavior } from './Behaviors/Toolbar/ToolbarBehavior'


do we also need a ToolbarButton behavior? Menu.Item renders a link with role='menuitem'. Can you please test with screen readers as well?

Aria does not specify any custom role for the menu items themselves when it comes to toolbar. only on the container itself.

However in the example https://www.w3.org/TR/wai-aria-practices/examples/toolbar/toolbar.html they use Buttons as menu items, and we use links by default. It is the reason I used component as child and it was fine, <li> then <button>.
But now, with latest master, I have an anchor tag that has a button as child. So it's <li> <a> <button> which means double focus. And it's kind of incorrect as far as semantics are concerned.
I've tried a trick, and replaced Buttons with Icons, and it works. Icons now are acting as anchors. Should we leave it like this?

Another way is to override the anchor tag from the rendering MenuItem altogether and replace it with the button. Don't know if it's possible right now. I can check. What do you think?

jurokapsiar · 2018-08-14T10:43:55Z

docs/src/examples/components/Menu/Variations/index.tsx

@@ -49,6 +49,16 @@ const Variations = () => (
      description="A vertical menu may take the size of its container."
      examplePath="components/Menu/Variations/MenuExampleVerticalFluid"
    />
+    <ComponentExample
+      title="Menu as a Tab List"
+      description="A menu that acts as a tablist."


let's change the description to "A menu with TabList accessibility behavior" and the one below to "A menu with toolbar accessibility behavior"

jurokapsiar · 2018-08-14T10:45:35Z

src/components/Menu/Menu.tsx

@@ -46,6 +46,9 @@ class Menu extends AutoControlledComponent<any, any> {

    /** Accessibility behavior if overridden by the user. */
    accessibility: PropTypes.oneOfType([PropTypes.object, PropTypes.func]),
+
+    /** Accessibility label that will be added by the accessibility behavior. */
+    'aria-label': PropTypes.string,


do we still need this? and line 71

codecov · 2018-08-15T09:09:24Z

Codecov Report

Merging #83 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #83   +/-   ##
=======================================
  Coverage   67.82%   67.82%           
=======================================
  Files         101      101           
  Lines        1386     1386           
  Branches      275      265   -10     
=======================================
  Hits          940      940           
  Misses        443      443           
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Menu/MenuItem.tsx	`92.3% <ø> (ø)`	⬆️
src/index.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57eb518...61bff62. Read the comment docs.

kuzhelov · 2018-08-22T18:13:04Z

@silviuavram, may I, please, kindly ask you to update the description of this PR, so that it would be easier to onboard more reviewers :) Thanks!

jurokapsiar · 2018-08-28T10:42:17Z

src/components/Menu/MenuItem.tsx

@@ -31,6 +31,9 @@ class MenuItem extends UIComponent<any, any> {
    /** Shorthand for primary content. */
    content: customPropTypes.contentShorthand,

+    /** A button can show it is currently unable to be interacted with. */


button -> menu item

copy-paste done wrong. will fix it.

src/components/Menu/MenuItem.tsx

src/lib/renderComponent.tsx

kuzhelov · 2018-08-30T07:51:43Z

test/specs/components/Menu/MenuItem-test.tsx

+
+      const disabledFlags = [true, false]
+      disabledFlags.forEach(disabledValue => {
+        test(`aria-disabled should be ${disabledValue} if button disabled prop is ${disabledValue}`, () => {


seems that there is a error here, as we are setting disabled for Menu, not for Button

kuzhelov

we need to address problem of coupling rendering function with accessibility - lets rethink originally taken 'handledProps' approach, see related comment

kuzhelov · 2018-09-03T09:58:44Z

src/lib/accessibility/Behaviors/Toolbar/ToolbarButtonBehavior.ts

+      'aria-labelledby': props['aria-labelledby'],
+    },
+  },
+  handledProps: ['aria-label', 'aria-labelledby'],


aria-disabled should be declared as handled one as well, related change to anchor's attribute value should be introduced:

anchor: { ... 'aria-disabled': props['disabled'] || props['aria-disabled'], ... },

kuzhelov · 2018-09-03T10:00:33Z

src/lib/accessibility/Behaviors/Tab/TabBehavior.ts

+      'aria-controls': props['aria-controls'],
+    },
+  },
+  handledProps: ['aria-label', 'aria-labelledby', 'aria-controls'],


aria-selected should be introduced as handled one, corresponding changes to attributes logic should be provided as well:

anchor: { ... 'aria-selected': props['active'] || props['aria-selected'], ... },

…st-ui/react into siavram-tablist-behaviors

jurokapsiar · 2018-09-03T13:10:52Z

src/lib/accessibility/Behaviors/Menu/MenuItemBehavior.ts

    },
  },
+  handledProps: ['aria-label', 'aria-labelledby', 'aria-expanded'],


please remove aria-expanded if you are removing it from anchor part

I missed this, wanted at first to add the same override as in tab and toolbar but I found that submenuOpened is probably a leftover as it's not being used. will remove this as well

mnajdova · 2018-09-04T13:37:48Z

docs/src/examples/components/Menu/Variations/MenuExampleTabList.shorthand.tsx

+  { key: 'editorials', content: 'Editorials', accessibility: TabBehavior },
+  { key: 'review', content: 'Reviews', accessibility: TabBehavior },
+  { key: 'events', content: 'Upcoming Events', accessibility: TabBehavior },
+]


Should the accessibility prop always be the same across all MenuItems inside one Menu? If this is the case, we can just apply the accessibility prop to the Menu and provide the same as defaultProperty to all MenuItems inside the Menu. Take a look into the renderItem method in the Menu and decide if we should do this in the similar way as the other common props are delegated to the MenuItem components.

yes, agree - we've discussed this moment, and agreed that it will be addressed by means of separate PR. For sure, there shouldn't be a mandatory requirement for consumer to specify this thing for menu items (especially given that those menu item objects could come as result of deserialising some HTTP response data, and at that moment there could be no context provided for them about which behavior Menu is using).

The way it could be achieved is by introducing behaviors property to Accessibility Behavior:

const behavior = { attributes: { .. }, behaviors: { item: TabBehavior } }

We should create and address RFC (issue) for that.

That a good point! Just one thing to keep in mind, that we should leave the possibility to set different behaviors for Items (for e.g. case when one MenuItem has a submenu, others - don't).
So, in Menu, we can set default behaviors for items, unless a user has already set it.

yes - but this option is always opened to us, as it is still possible to explicitly specify behavior for each individual menu item via accessibility prop:

<Menu items=[ ... { key: '...', content: '...', accessibility: CustomBehavior } ]

Here we are touching just the fact that some reasonable default behavior should be applied to Menu/List items - without limiting the set of cases covered now.

kuzhelov · 2018-09-04T14:44:36Z

it would be great if @levithomason would take a look on this PR as well

docs/src/examples/components/Menu/Variations/MenuExampleToolbar.shorthand.tsx

src/components/Menu/MenuItem.tsx

silviuaavram requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 13, 2018 10:59

sophieH29 reviewed Aug 13, 2018

View reviewed changes

jurokapsiar reviewed Aug 14, 2018

View reviewed changes

silviuaavram force-pushed the siavram-tablist-behaviors branch from 313c4ea to d19b305 Compare August 14, 2018 09:26

jurokapsiar reviewed Aug 14, 2018

View reviewed changes

silviuaavram force-pushed the siavram-tablist-behaviors branch from 84cef80 to e1d93bd Compare August 21, 2018 08:59

silviuaavram requested a review from miroslavstastny as a code owner August 21, 2018 08:59

kuzhelov added needs author feedback Author's opinion is asked 🚧 WIP labels Aug 22, 2018

silviuaavram force-pushed the siavram-tablist-behaviors branch 2 times, most recently from 7729e06 to efc818c Compare August 28, 2018 09:58

jurokapsiar reviewed Aug 28, 2018

View reviewed changes

silviuaavram force-pushed the siavram-tablist-behaviors branch from 72d7e9e to 61206f3 Compare August 29, 2018 08:29

silviuaavram added 🚀 ready for review and removed needs author feedback Author's opinion is asked 🚧 WIP labels Aug 29, 2018

kuzhelov reviewed Aug 30, 2018

View reviewed changes

src/components/Menu/MenuItem.tsx Outdated Show resolved Hide resolved

kuzhelov reviewed Aug 30, 2018

View reviewed changes

src/lib/renderComponent.tsx Outdated Show resolved Hide resolved

kuzhelov reviewed Aug 30, 2018

View reviewed changes

kuzhelov suggested changes Aug 30, 2018

View reviewed changes

kuzhelov added the needs author changes Author needs to implement changes before merge label Aug 30, 2018

Silviu Avram added 3 commits August 30, 2018 16:15

worked around an issue caused by the fact that we don't use semicolons

0781982

added disabled prop to the menuitem interface

cf55fe1

corrected some test name errors

cf0c915

silviuaavram force-pushed the siavram-tablist-behaviors branch from 61206f3 to cf0c915 Compare August 31, 2018 12:42

use 'underlined' Menu prop intead of obolete 'shape'

8656435

kuzhelov reviewed Sep 3, 2018

View reviewed changes

Silviu Avram added 5 commits September 3, 2018 12:52

added aria attributes values as having greater priority in the behaviour

074f057

Merge branch 'siavram-tablist-behaviors' of https://github.com/stardu…

cb5f140

…st-ui/react into siavram-tablist-behaviors

Merge branch 'master' into siavram-tablist-behaviors

6eb5785

applied the same override strategy for menuItemBehavior

80a630f

performed correct prop check and added more unit tests for prop override

f44072c

jurokapsiar reviewed Sep 3, 2018

View reviewed changes

Silviu Avram added 2 commits September 3, 2018 16:42

removed leftover aria-expanded from handledProps

1167014

merged master and fixed renderComponent conflict

424277d

mnajdova reviewed Sep 4, 2018

View reviewed changes

kuzhelov approved these changes Sep 4, 2018

View reviewed changes

kuzhelov changed the title ~~Added behaviours for tabs, tablists and toolbars~~ feat(accessibility): add behaviours for tabs, tablists and toolbars Sep 4, 2018

kuzhelov added 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Sep 4, 2018

miroslavstastny reviewed Sep 4, 2018

View reviewed changes

docs/src/examples/components/Menu/Variations/MenuExampleToolbar.shorthand.tsx Outdated Show resolved Hide resolved

miroslavstastny reviewed Sep 4, 2018

View reviewed changes

src/components/Menu/MenuItem.tsx Show resolved Hide resolved

Silviu Avram added 2 commits September 5, 2018 10:11

Merge branch 'master' into siavram-tablist-behaviors

c62dc56

added incons as jsons in the examples

61bff62

silviuaavram mentioned this pull request Sep 5, 2018

Add MenuItem disabled prop #186

Closed

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 5, 2018

kuzhelov merged commit e56b62c into master Sep 5, 2018

kuzhelov deleted the siavram-tablist-behaviors branch November 19, 2018 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(accessibility): add behaviours for tabs, tablists and toolbars #83

feat(accessibility): add behaviours for tabs, tablists and toolbars #83

silviuaavram commented Aug 13, 2018 •

edited

Loading

sophieH29 Aug 13, 2018

jurokapsiar Aug 14, 2018

silviuaavram Aug 14, 2018

sophieH29 Aug 13, 2018

jurokapsiar Aug 14, 2018

silviuaavram Aug 14, 2018

jurokapsiar Aug 14, 2018

silviuaavram Aug 14, 2018

kuzhelov Sep 1, 2018

jurokapsiar Aug 14, 2018

silviuaavram Aug 14, 2018

jurokapsiar Aug 14, 2018

silviuaavram Aug 14, 2018 •

edited

Loading

jurokapsiar Aug 14, 2018

jurokapsiar Aug 14, 2018

codecov bot commented Aug 15, 2018 •

edited

Loading

kuzhelov commented Aug 22, 2018

jurokapsiar Aug 28, 2018

silviuaavram Aug 28, 2018

kuzhelov Aug 30, 2018

kuzhelov left a comment

kuzhelov Sep 3, 2018

kuzhelov Sep 3, 2018

jurokapsiar Sep 3, 2018

silviuaavram Sep 3, 2018

mnajdova Sep 4, 2018

kuzhelov Sep 4, 2018

sophieH29 Sep 4, 2018

kuzhelov Sep 5, 2018 •

edited

Loading

kuzhelov commented Sep 4, 2018

feat(accessibility): add behaviours for tabs, tablists and toolbars #83

feat(accessibility): add behaviours for tabs, tablists and toolbars #83

Conversation

silviuaavram commented Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silviuaavram Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2018 • edited Loading

Codecov Report

kuzhelov commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

kuzhelov commented Sep 4, 2018

silviuaavram commented Aug 13, 2018 •

edited

Loading

silviuaavram Aug 14, 2018 •

edited

Loading

codecov bot commented Aug 15, 2018 •

edited

Loading

kuzhelov Sep 5, 2018 •

edited

Loading