-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document outline: Use links not buttons #10815
Conversation
490e55e
to
b743cec
Compare
3bf8c41
to
8654dfd
Compare
8654dfd
to
1ccfe6f
Compare
|
@aduth can you please give this another review? also appreciate any tips on getting the tests to pass. |
Tests fail because block client ID is not deterministic. Prior art: gutenberg/test/integration/full-content/full-content.spec.js Lines 59 to 60 in 4369d93
Maybe not testing snapshots
|
# Conflicts: # packages/editor/src/components/document-outline/index.js # packages/editor/src/components/document-outline/item.js # packages/editor/src/components/table-of-contents/index.js # packages/editor/src/components/table-of-contents/panel.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise, this appears all good.
It seems we have unsettled yet:
- What difference this actually makes
- What are the plans for the block navigation menu, whose behavior is now distinct from the document outline as of these changes
# Conflicts: # packages/editor/src/components/document-outline/item.js
This is pretty clearly described in #7142 In particular improves the expected behavior of the document outline for blind users who access the outline using a screen reader: As a blind user of a screen reader, I would expect that hyperlinks lead to somewhere (and the focus will be moved there), and I would expect the behaviour of buttons to be less predictable in general. Changing the buttons to links helps users understand what will happen when they are clicked - especially blind users who can't see the visual styling the buttons have been given.
I agree, this change does point to a potential issue with the block navigation menu's accessibility. For now, I've kept the focus of this PR to the document outline. |
|
* Adjust document outline to use an a tag vs button * target href links directly to page anchors, remove onClick handler * update test snapshot * update snapshot * better titleNode targeting * update snapshot * update snapshot * Close the table of contents panel when a link is clicked * add deterministic block id to tests * remove redundant screen reader text * Adjust map to avoid mutating original object * update snapshot * Update packages/editor/src/components/document-outline/index.js Co-Authored-By: adamsilverstein <adam@10up.com> * Update packages/editor/src/components/document-outline/test/index.js remove leading _ Co-Authored-By: adamsilverstein <adam@10up.com> * Update packages/editor/src/components/document-outline/index.js Co-Authored-By: adamsilverstein <adam@10up.com> * update snapshot * update snapshots * update snapshot * update snapshots * fix up e2e tests * Fix snapshots by removing single quotes from outline links * Update packages/editor/src/components/document-outline/index.js Co-Authored-By: adamsilverstein <adam@10up.com> * change target to href * rename close -> closeOutline * update snapshots after property name changes * rename close/onClose -> onRequestClose for TOC, on * restore onSelect * complete renaming * Block links are only valid for the current session - remove hash after following * update snapshot * cleanup; move block id hash removal functionality up to document outline; now includes title * update snapshot * use replaceState vs pushState * use defer and import at top of file * removeURLHash as helper * remove passing event in select handler * improve doc block * Skip title in outline when title node not found * remove removeURLHash
* Adjust document outline to use an a tag vs button * target href links directly to page anchors, remove onClick handler * update test snapshot * update snapshot * better titleNode targeting * update snapshot * update snapshot * Close the table of contents panel when a link is clicked * add deterministic block id to tests * remove redundant screen reader text * Adjust map to avoid mutating original object * update snapshot * Update packages/editor/src/components/document-outline/index.js Co-Authored-By: adamsilverstein <adam@10up.com> * Update packages/editor/src/components/document-outline/test/index.js remove leading _ Co-Authored-By: adamsilverstein <adam@10up.com> * Update packages/editor/src/components/document-outline/index.js Co-Authored-By: adamsilverstein <adam@10up.com> * update snapshot * update snapshots * update snapshot * update snapshots * fix up e2e tests * Fix snapshots by removing single quotes from outline links * Update packages/editor/src/components/document-outline/index.js Co-Authored-By: adamsilverstein <adam@10up.com> * change target to href * rename close -> closeOutline * update snapshots after property name changes * rename close/onClose -> onRequestClose for TOC, on * restore onSelect * complete renaming * Block links are only valid for the current session - remove hash after following * update snapshot * cleanup; move block id hash removal functionality up to document outline; now includes title * update snapshot * use replaceState vs pushState * use defer and import at top of file * removeURLHash as helper * remove passing event in select handler * improve doc block * Skip title in outline when title node not found * remove removeURLHash
Description
Fixes #7142.
Anchor tags are not being used to structure the Document Outline functionality. Anchor tags are meant to lead to information on the same page or on another page and are the most suitable element to be used in this section.
We are emulating the hyperlink behaviour by moving keyboard (and viewport) focus to the relevant area, but the behaviour may be unpredictable to blind users (even with the help text).
How has this been tested?
After making the changes, I tested that the document outlined looked correct (the same) and clicking the links worked as expected. One slight difference here is that hovering over the items now shows a blue link color. This seems appropriate here, noting because it is a change. I also tested with Voice Over to review spoken messages which seemed good.
Screenshots
Types of changes
Checklist: