Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Accessibility): Make Grid keyboard navigable by adding Grid Behavior #398

Merged
merged 12 commits into from
Nov 5, 2018

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Oct 24, 2018

This PR adds a possibility to navigate Grid items with arrow buttons.
What was added:

  • extended Grid's API, so now it can process accessibility prop. (though, a default value is defaultBehavior)
  • added gridBehavior with FocusZone circular bi-directional navigation
  • added example of keyboard navigable Grid component, which uses gridBehavior for that purpose.

TODO:

  • update CHANGELOG.md

@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@74f3e11). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #398   +/-   ##
=========================================
  Coverage          ?   91.79%           
=========================================
  Files             ?       41           
  Lines             ?     1328           
  Branches          ?      193           
=========================================
  Hits              ?     1219           
  Misses            ?      105           
  Partials          ?        4
Impacted Files Coverage Δ
src/index.ts 100% <100%> (ø)
src/components/Grid/Grid.tsx 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 74f3e11...c22ae4d. Read the comment docs.

@sophieH29 sophieH29 changed the title Make Grid keyboard navigable by adding Grid Behavior feat(Accessibility): Make Grid keyboard navigable by adding Grid Behavior Oct 24, 2018
Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

looks good generally, please see comments

import { Grid, Image, Button, gridBehavior } from '@stardust-ui/react'
import _ from 'lodash'

const images = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you make this an array to save space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, sure, that does make sense. Thank you!

import { Grid, Image, Button, gridBehavior } from '@stardust-ui/react'
import _ from 'lodash'

const images = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you make this an array to save space?

@bmdalex
Copy link
Collaborator

bmdalex commented Oct 26, 2018

I was trying this in our react conf prototype and it does not behave correctly when the grid items contain focusable items themselves:

screen shot 2018-10-26 at 13 58 16

As you can see in the screenshot above, the grid items contain a button ("FULL BIO") so navigation works incorrectly:

actual: arrow keys navigate to grid items then to buttons
expected: two level of keyboard navigation:

  • arrow keys navigate between grid items;
  • enter key navigates inside the grid item (level 2 key nav) that will trap focus;
  • esc key navigates back to the grid item (level 1 key nav)

fyi @sophieH29 @jurokapsiar

@sophieH29
Copy link
Contributor Author

sophieH29 commented Oct 26, 2018

@Bugaa92 You are totally right about having nested navigation. But it is not what expected in current PR. Current PR provides navigation with arrow keys on grid items itself, whether they are focusable elements or have set data-is-focusable=true

@bmdalex
Copy link
Collaborator

bmdalex commented Nov 2, 2018

in the example I provided above, each item contains a button but also data-is-focusable=true at the top item element which causes weird navigation:

<Grid
  <div className="grid item" data-is-focusable="true" >
    <button>FULL BIO</button>
  </div>
  <div className="grid item" data-is-focusable="true" >
    <button>FULL BIO</button>
  </div>
  ...
  <div className="grid item" data-is-focusable="true" >
    <button>FULL BIO</button>
  </div>
/>

let's say the focus is in the Grid; user scenario:

  • focus is on first grid item;
  • press right arrow key; focus goes to button inside first grid item;
  • press right arrow key; focus goes to second grid item;
  • press right arrow key; focus goes to button inside second grid item;
  • etc.

This looks wrong to me, do we want merge it like this?

LATER EDIT:

Discussed with @sophieH29 and all these complex cases will be handled in future work

@sophieH29 sophieH29 merged commit 83e933b into master Nov 5, 2018
@sophieH29 sophieH29 deleted the feat/grid-a11y-behavior branch November 5, 2018 10:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants