Add a11y support for the tree-control #36459

mdperez86 · 2023-01-16T21:10:11Z

Depends on #36590

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Depends on #36480
Partialy closes #35851

This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Run storybook
Visit http://localhost:6007/?path=/story/woocommerce-admin-experimental-treecontrol--selection-multiple
This implementation follow the a11y guides described in https://www.w3.org/WAI/ARIA/apg/patterns/treeview/examples/treeview-navigation/.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2023-01-16T21:42:25Z

Test Results Summary

Commit SHA: ef34a9b

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 54s
E2E Tests	189	0	0	6	0	195	16m 0s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

… custom logic

…checkbox control

… custom logic

codecov · 2023-03-01T18:09:44Z

Codecov Report

Merging #36459 (ef34a9b) into trunk (ac47d18) will decrease coverage by 0.0%.
The diff coverage is 24.3%.

Additional details and impacted files

@@            Coverage Diff             @@
##             trunk   #36459     +/-   ##
==========================================
- Coverage     46.7%    46.7%   -0.0%     
- Complexity   17183    17188      +5     
==========================================
  Files          429      429             
  Lines        64799    64820     +21     
==========================================
  Hits         30251    30251             
- Misses       34548    34569     +21

Impacted Files	Coverage Δ
...ocommerce/includes/admin/class-wc-admin-assets.php	`0.0% <0.0%> (ø)`
...mmerce/includes/admin/class-wc-admin-importers.php	`0.0% <0.0%> (ø)`
plugins/woocommerce/includes/class-wc-ajax.php	`4.3% <0.0%> (-0.1%)`	⬇️
...s/Version1/class-wc-rest-coupons-v1-controller.php	`42.9% <0.0%> (ø)`
...rs/Version1/class-wc-rest-orders-v1-controller.php	`46.2% <0.0%> (ø)`
...rters/class-wc-product-csv-importer-controller.php	`36.1% <28.6%> (ø)`
.../includes/import/class-wc-product-csv-importer.php	`74.9% <44.4%> (ø)`
...s/Version2/class-wc-rest-coupons-v2-controller.php	`94.7% <75.0%> (ø)`
plugins/woocommerce/includes/class-wc-coupon.php	`73.9% <100.0%> (ø)`
...lugins/woocommerce/includes/class-wc-discounts.php	`84.5% <100.0%> (ø)`
... and 18 more

nathanss

Nice job. It's testing well on the storybook and the aria attributes seem correct.

I'm just wondering if there wasn't a way of doing this without relying on CSS and DOM manipulation, but I don't have any suggestions regarding that, so I just left a small suggestion.

nathanss · 2023-03-01T18:10:11Z

packages/js/components/src/experimental-tree-control/hooks/use-keyboard.ts

+		return null;
+	}
+
+	const step = {


I think you can move this to outside the function as it's a constant

I put it there because I intentionally wanted to hide the step variable from being used outside of that function.

But I don't have a strong opinion for this anyway. It's done now!

mattsherman · 2023-03-01T21:55:02Z

I'm just wondering if there wasn't a way of doing this without relying on CSS and DOM manipulation, but I don't have any suggestions regarding that, so I just left a small suggestion.

@nathanss What specific CSS and DOM manipulation are you referring to?

mattsherman · 2023-03-01T22:01:02Z

packages/js/components/changelog/add-35851-tree-control-a11y

+Significance: minor
+Type: dev
+
+Add a11y support for the tree-control


Instead of tree-control, consider Tree component

Thanks @mattsherman. It's done now.

nathanss · 2023-03-01T22:58:38Z

@nathanss What specific CSS and DOM manipulation are you referring to?

@mattsherman I guess it's not "manipulation" since we're not changing the values, just querying them. I'm referring to all the document queries inside the use-keyboard on getFirstChild, getFirstAncestor, getAllHeadings, and getNextFocusableElement. It's pretty philosophical but it seems we do that a lot in our code instead of doing it the "React way". Am I making sense?

mattsherman · 2023-03-02T13:38:18Z

@nathanss What specific CSS and DOM manipulation are you referring to?

@mattsherman I guess it's not "manipulation" since we're not changing the values, just querying them. I'm referring to all the document queries inside the use-keyboard on getFirstChild, getFirstAncestor, getAllHeadings, and getNextFocusableElement. It's pretty philosophical but it seems we do that a lot in our code instead of doing it the "React way". Am I making sense?

Thanks for the clarification. I can see how these DOM queries could be non-ideal, as if the rendered DOM were changed elsewhere in code, these queries would need to be changed.

What would a more "React way" look like to you?

nathanss · 2023-03-02T13:47:33Z

packages/js/components/src/experimental-tree-control/hooks/use-keyboard.ts

+	currentHeading: HTMLDivElement
+): NodeListOf< HTMLDivElement > | undefined {
+	const rootTree = currentHeading.closest< HTMLDivElement >(
+		'.experimental-woocommerce-tree--level-1'


Is that -1 here correct? Can't that number change depending on the situation?

Is that -1 here correct?

Yes it's. This --level-1 refers to the nesting level of the tree. So that all subtrees have their parent level + 1 like --level-2, --level-3 ... going from the root to the leafs.

nathanss · 2023-03-02T14:19:38Z

What would a more "React way" look like to you?

@mattsherman I guess the main thing that CSS is solving in use-keyboard is fetching references to other Tree Items inside one of the Tree Items to focus them (and in some cases to expand and collapse through the useExpander hook). The alternative could be a shared context between tree items that would be able to update a state that would trigger the necessary changes based only on the tree data, instead of CSS.

For the focus, each tree item could have a useEffect hook that would focus itself based on some change in this context.

This could potentially make the performance worse. Maybe it's OK to rely on CSS a little bit, especially in a component's implementation. This all seems to be encapsulated inside the TreeControl, so at least we have full control of the CSS. To be fair I'm more accustomed to using react components instead of implementing them 😄

mdperez86 · 2023-03-02T15:39:55Z

What would a more "React way" look like to you?

@mattsherman I guess the main thing that CSS is solving in use-keyboard is fetching references to other Tree Items inside one of the Tree Items to focus them (and in some cases to expand and collapse through the useExpander hook). The alternative could be a shared context between tree items that would be able to update a state that would trigger the necessary changes based only on the tree data, instead of CSS.

For the focus, each tree item could have a useEffect hook that would focus itself based on some change in this context.

This could potentially make the performance worse. Maybe it's OK to rely on CSS a little bit, especially in a component's implementation. This all seems to be encapsulated inside the TreeControl, so at least we have full control of the CSS. To be fair I'm more accustomed to using react components instead of implementing them 😄

Yes that was my first approach, trying to use a context but the tree is a complex structure that we should use carefully. After trying to perform across tree-items operations thinks becomes heavy to manage. Like focusing the first child of an expanded subtree, or focus the first ancestor or successor, ... Also, the idea was to keep thinks scoped to each own custom hook so that we can replace them easily when needed, another reason why the hooks are very isolated. Well, if we could find a way to deal with all the scenarios without impacting performance, I'm definitely in!

nathanss

I'm OK with the current code and it seems to work well, so I'm approving it

mdperez86 added focus: product management Related to product creation and editing. focus: new product ux revamped product management experience labels Jan 16, 2023

mdperez86 requested a review from a team January 16, 2023 21:10

mdperez86 self-assigned this Jan 16, 2023

github-actions bot added the package: @woocommerce/components issues related to @woocommerce/components label Jan 16, 2023

mdperez86 marked this pull request as ready for review January 17, 2023 13:52

mdperez86 force-pushed the add/35851-tree-control-a11y branch from 2b9739d to e5a813a Compare January 17, 2023 19:03

mattsherman assigned mattsherman and unassigned mdperez86 Jan 19, 2023

mattsherman mentioned this pull request Jan 24, 2023

[Epic] TreeControl component #36590

Closed

7 tasks

mattsherman added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Jan 24, 2023

mattsherman marked this pull request as draft February 1, 2023 19:58

mattsherman removed their assignment Feb 15, 2023

AnnaMag marked this pull request as ready for review February 23, 2023 14:53

octaedro requested review from octaedro and a team and removed request for a team and octaedro February 24, 2023 17:34

mdperez86 self-assigned this Feb 28, 2023

mdperez86 added 8 commits March 1, 2023 14:19

Add tree-control expand/collapse on click the expander button or by a…

d990ea6

… custom logic

Upgrade WP components dependency to v19.8.5 to support indeterminate …

3a9a66a

…checkbox control

Add styles to fit the disign

9d0ae86

Add tree-control expand/collapse on click the expander button or by a…

4ece220

… custom logic

Add a11y landmarks

132ffad

Add keyboard interactions

e98599b

Add changelog file

58bd837

Fix linter errors

7fd9f31

mdperez86 force-pushed the add/35851-tree-control-a11y branch from e5a813a to 7fd9f31 Compare March 1, 2023 17:42

Resolving rebase conflicts

74b49c0

mdperez86 force-pushed the add/35851-tree-control-a11y branch from 476a3f9 to 74b49c0 Compare March 1, 2023 17:48

mdperez86 requested review from octaedro and nathanss March 1, 2023 17:51

nathanss requested changes Mar 1, 2023

View reviewed changes

mattsherman reviewed Mar 1, 2023

View reviewed changes

nathanss reviewed Mar 2, 2023

View reviewed changes

Add comment suggestions

ef34a9b

mdperez86 requested review from nathanss and mattsherman March 2, 2023 15:40

nathanss approved these changes Mar 2, 2023

View reviewed changes

mdperez86 merged commit 7dd3797 into trunk Mar 2, 2023

mdperez86 deleted the add/35851-tree-control-a11y branch March 2, 2023 20:26

github-actions bot added this to the 7.6.0 milestone Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a11y support for the tree-control #36459

Add a11y support for the tree-control #36459

mdperez86 commented Jan 16, 2023 •

edited

github-actions bot commented Jan 16, 2023 •

edited

codecov bot commented Mar 1, 2023 •

edited

nathanss left a comment

nathanss Mar 1, 2023

mdperez86 Mar 2, 2023

mattsherman commented Mar 1, 2023

mattsherman Mar 1, 2023

mdperez86 Mar 2, 2023

nathanss commented Mar 1, 2023 •

edited

mattsherman commented Mar 2, 2023

nathanss Mar 2, 2023

mdperez86 Mar 2, 2023

nathanss commented Mar 2, 2023

mdperez86 commented Mar 2, 2023

nathanss left a comment

Add a11y support for the tree-control #36459

Add a11y support for the tree-control #36459

Conversation

mdperez86 commented Jan 16, 2023 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

github-actions bot commented Jan 16, 2023 • edited

Test Results Summary

codecov bot commented Mar 1, 2023 • edited

Codecov Report

nathanss left a comment

Choose a reason for hiding this comment

nathanss Mar 1, 2023

Choose a reason for hiding this comment

mdperez86 Mar 2, 2023

Choose a reason for hiding this comment

mattsherman commented Mar 1, 2023

mattsherman Mar 1, 2023

Choose a reason for hiding this comment

mdperez86 Mar 2, 2023

Choose a reason for hiding this comment

nathanss commented Mar 1, 2023 • edited

mattsherman commented Mar 2, 2023

nathanss Mar 2, 2023

Choose a reason for hiding this comment

mdperez86 Mar 2, 2023

Choose a reason for hiding this comment

nathanss commented Mar 2, 2023

mdperez86 commented Mar 2, 2023

nathanss left a comment

Choose a reason for hiding this comment

mdperez86 commented Jan 16, 2023 •

edited

github-actions bot commented Jan 16, 2023 •

edited

codecov bot commented Mar 1, 2023 •

edited

nathanss commented Mar 1, 2023 •

edited