feat(ObjectViewer): improve a11y #1616

jsomsanith-tlnd · 2018-08-13T15:59:18Z

What is the problem this PR is trying to solve?
Table mode

no caption
no ids on headers
no headers attributes on cells

JSON mode

no accessible tree structure
no tree gesture

What is the chosen solution to this problem?
Table mode

add hidden caption that will contains a props.title (required)
add ids on headers based on a props.id (required)
set those ids in the cells headers attribute

JSON mode (refacto)

follow https://www.w3.org/TR/wai-aria-practices-1.1/examples/treeview/treeview-2/treeview-2a.html
extract gesture from Treeview into a new HOC, used in ObjectViewer and TreeView

Please check if the PR fulfills these requirements

The PR commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features) And non reg done before need review
Docs have been added / updated (for bug fixes / features)
Related design / discussions / pages (not in jira), if any, are all linked or available in the PR

[x] This PR introduces a breaking change

This impacts only the component. If you use the cmf container, it works as expected.

Before	After	Explanation
`props.onMouseOver`	-	Not accessible. This would introduce performance issues with keyboad navigation. But this is not used, so let's remove it.
`props.onSelect(event, jsonpath)`	`props.onSelect(event, { jsonpath })`	This simplifies the code. This is triggered through TreeGesture HOC that takes an item and pass it through to the props callback. This is simple and compatible with TreeView and the new ObjectViewer apis.
-	Required `props.onToggleAllSiblings(event, siblings)`	This is for accessibility. It is triggered on `*` keydown to expand all the nodes at the same level
has been removed.

Before

import ObjectViewer from '@talend/react-components/lib/ObjectViewer';

class MyComponent extends React.Component {
	...

	mouseOverHandler(event, item) {}

	selectHandler(event, jsonpath) {}

	render() {
		return (
			<ObjectViewer
				onMouseOver={this.mouseOverHandler}
				onSelect=(this.selectHandler}
				...
			/>
		);
	}
}

After

import ObjectViewer from '@talend/react-components/lib/ObjectViewer';

class MyComponent extends React.Component {
	...

	selectHandler(event, jsonpath) {}

	toggleAllHandler(event, itemsToToggle) {}

	render() {
		return (
			<ObjectViewer
				onSelect=(this.selectHandler}
				onToggleAllSiblings={this.toggleAllHandler}
				...
			/>
		);
	}
}

build-travis-ci · 2018-08-13T16:38:02Z

Demo is available here

build-travis-ci · 2018-08-13T18:15:19Z

Demo is available here

build-travis-ci · 2018-08-14T13:33:30Z

Demo is available here

build-travis-ci · 2018-08-14T16:57:09Z

Demo is available here

build-travis-ci · 2018-08-14T17:32:18Z

Demo is available here

build-travis-ci · 2018-08-16T08:08:44Z

Demo is available here

build-travis-ci · 2018-08-16T08:32:42Z

Demo is available here

build-travis-ci · 2018-08-16T10:17:44Z

Demo is available here

build-travis-ci · 2018-08-16T10:34:03Z

Demo is available here

build-travis-ci · 2018-08-16T14:02:04Z

Demo is available here

build-travis-ci · 2018-08-16T14:37:35Z

Demo is available here

jmfrancois

I have covered the treeview part.
Don t forget to check the console in the storybook i have lots of JS Error

jmfrancois · 2018-09-13T09:41:27Z

packages/components/src/Tree/WithTreeGesture.component.js

+				case keycode.codes.end:
+					focusOn(event, getLastItem(ref));
+					break;
+				case keycode.codes['numpad *']:


it doesn t work with *

Fixed it, but I need to test the key, not the keyCode. Depending on the keyboard, the keyCode is different :/

jmfrancois · 2018-09-13T11:24:39Z

packages/components/src/Tree/WithTreeGesture.component.js

+
+	TreeGesture.propTypes = {
+		...omit(WrappedComponent.propTypes, 'onKeyDown'),
+		onToggleAllSiblings: PropTypes.func.isRequired,


onSelect and onToggle are used and required (in your code)

jmfrancois · 2018-09-13T11:25:28Z

packages/components/src/Tree/WithTreeGesture.component.js

+					break;
+				default:
+					break;
+			}


please add support for this.props.onKeyDown

No there is no need. If the need comes, it's easy to add it, but adding a feature in case someone needs it is not a good idea IMO

jmfrancois · 2018-09-13T11:25:53Z

packages/components/src/Tree/WithTreeGesture.component.js

@@ -0,0 +1,162 @@
+import React from 'react';


it s a function so please name the file withTreeGesture (like connect)

Changed the file name

jmfrancois · 2018-09-13T11:28:10Z

packages/components/src/Tree/WithTreeGesture.component.test.js

+
+const treeProps = {
+	items: [
+		{ id: 0 }, // 0 0


sorry I don t understand those comments ( // 1 0)
Looks like battleships coordinates :)

They are coordinates :D
Those are level / index. It's easier to understand with those while reading some tests. I'll add a legend

jmfrancois · 2018-09-13T11:37:01Z

packages/components/src/Tree/WithTreeGesture.component.js

+
+		onToggleAllSiblings(event, items) {
+			event.stopPropagation();
+			return this.props.onToggleAllSiblings(event, items);


you have hide event.stopPropagation in every handler.
I found that makes the readability quite hard.
What do you think about:
call event.stopPropagation in onKeyDown when the key is handled and call the associated effect.

the focusOn should not stop propagation but just call focus if element exists.

jmfrancois · 2018-09-13T11:45:36Z

packages/components/src/Tree/WithTreeGesture.component.test.js

+			],
+		}, // 0 1
+		{ id: 2 }, // 0 2
+		{ id: 3 }, // 0 3


not sure if we should keep those comments

I added the legend

build-travis-ci · 2018-09-13T15:20:39Z

Demo is available here

build-travis-ci · 2018-09-14T08:21:59Z

Demo is available here

build-travis-ci · 2018-09-14T08:47:30Z

Demo is available here

build-travis-ci · 2018-09-14T08:58:09Z

Demo is available here

jmfrancois · 2018-09-17T13:17:25Z

packages/components/src/Tree/withTreeGesture.test.js

+		};
+		const wrapper = mount(<ComponentWithGesture {...props} />);
+		const event = {
+			keyCode: keycode.codes['numpad *'],


this is wrong, the code use the native event, not the numpad, so why it here ?

jmfrancois · 2018-09-17T13:17:41Z

packages/components/stories/TreeView.js

@@ -154,6 +154,7 @@ const defaultProps = {
 	structure,
 	onSelect: action('onSelect'),
 	onToggle: action('onToggle'),
+	onToggleAllSiblings: action('onToggleAllSiblings'),


build-travis-ci · 2018-09-17T14:20:17Z

Demo is available here

build-travis-ci · 2018-09-17T14:32:00Z

Demo is available here

build-travis-ci · 2018-09-18T07:37:12Z

Demo is available here

This reverts commit 99ba3f3.

feat(ObjectViewer): improve a11y

ae8cb0a

jsomsanith-tlnd added frontend work in progress labels Aug 13, 2018

test(ci): update code style outputs

1010a8d

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

6f0d9f7

jsomsanith-tlnd and others added 3 commits August 14, 2018 18:25

Continue refactor

2b5fa59

test(ci): update code style outputs

d2e439c

test(ci): prettier

c01750a

jsomsanith-tlnd and others added 2 commits August 16, 2018 10:01

Fix ellipsis

92227d0

test(ci): update code style outputs

7ec39fa

jsomsanith-tlnd and others added 2 commits August 16, 2018 11:44

Add text alternatives to possible gestures

e5e3166

test(ci): update code style outputs

730a802

jsomsanith-tlnd added 2 commits August 16, 2018 15:37

Clean style

d56ee41

Tests

776982a

jsomsanith-tlnd added need review easy review and removed work in progress labels Aug 16, 2018

jsomsanith-tlnd and others added 2 commits August 16, 2018 15:42

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

e3c1655

test(ci): update code style outputs

78f40da

jmfrancois suggested changes Sep 13, 2018

View reviewed changes

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

0f0f7dd

Fix * gesture

c500007

jsomsanith-tlnd and others added 2 commits September 14, 2018 10:37

Fixes after review

42c42a9

chore(ci): update code style outputs

5e1e42c

Fix

0644ef1

jmfrancois suggested changes Sep 17, 2018

View reviewed changes

jsomsanith-tlnd and others added 3 commits September 17, 2018 16:09

Fix after review

c0f972b

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

71f224d

chore(ci): update code style outputs

a177add

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

7086d27

jmfrancois approved these changes Sep 18, 2018

View reviewed changes

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

286c472

Merge branch 'master' into jsomsanith/feat/a11y_object_viewer

e6f1a98

jsomsanith-tlnd merged commit 42b7ac8 into master Sep 18, 2018

jsomsanith-tlnd deleted the jsomsanith/feat/a11y_object_viewer branch September 18, 2018 08:26

jmfrancois added a commit that referenced this pull request Sep 24, 2018

Revert "feat(ObjectViewer): improve a11y (#1616)"

99ba3f3

jsomsanith-tlnd added a commit that referenced this pull request Sep 24, 2018

feat(ObjectViewer): improve a11y (#1616)

5c7f534

This reverts commit 99ba3f3.

This was referenced Sep 24, 2018

feat(ObjectViewer): improve a11y (#1616) #1712

Merged

fix(ObjectViewer): select and toggle by click #1713

Merged

jsomsanith-tlnd added a commit that referenced this pull request Sep 25, 2018

feat(ObjectViewer): improve a11y (#1616) (#1712)

5264e57

jsomsanith-tlnd pushed a commit that referenced this pull request Sep 25, 2018

Revert "feat(ObjectViewer): improve a11y (#1616)"

a7ebbb6

jsomsanith-tlnd added a commit that referenced this pull request Sep 25, 2018

feat(ObjectViewer): improve a11y (#1616) (#1712)

a4cb53e

feat(ObjectViewer): improve a11y #1616

feat(ObjectViewer): improve a11y #1616

Conversation

jsomsanith-tlnd commented Aug 13, 2018 • edited Loading

build-travis-ci commented Aug 13, 2018

build-travis-ci commented Aug 13, 2018

build-travis-ci commented Aug 14, 2018

build-travis-ci commented Aug 14, 2018

build-travis-ci commented Aug 14, 2018

build-travis-ci commented Aug 16, 2018

build-travis-ci commented Aug 16, 2018

build-travis-ci commented Aug 16, 2018

build-travis-ci commented Aug 16, 2018

build-travis-ci commented Aug 16, 2018

build-travis-ci commented Aug 16, 2018

jmfrancois 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

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

build-travis-ci commented Sep 13, 2018

build-travis-ci commented Sep 14, 2018

build-travis-ci commented Sep 14, 2018

build-travis-ci commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

build-travis-ci commented Sep 17, 2018

build-travis-ci commented Sep 17, 2018

build-travis-ci commented Sep 18, 2018

jsomsanith-tlnd commented Aug 13, 2018 •

edited

Loading