Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Review Navigation Treeview Example Using Declared Properties #226

Closed
5 tasks done
mcking65 opened this issue Dec 14, 2016 · 21 comments
Closed
5 tasks done

Review Navigation Treeview Example Using Declared Properties #226

mcking65 opened this issue Dec 14, 2016 · 21 comments
Labels
editorial Changes to prose that don't alter intended meaning, e.g., phrasing, grammar. May fix inaccuracies.
Milestone
Jan 2017 Clean Up

Comments

@mcking65
Copy link
Contributor

mcking65 commented Dec 14, 2016 •
edited
Loading

This issue is for collecting feedback on and tracking editorial revisions to the
Navigation Treeview Example Using Declared Properties.

Required Updates
@mcking65 mcking65 added editorial Changes to prose that don't alter intended meaning, e.g., phrasing, grammar. May fix inaccuracies. needs edits labels Dec 14, 2016
@mcking65 mcking65 added this to the 1.1 PR milestone Dec 14, 2016
@carmacleod
Copy link
Contributor

Typo: compuuted

@carmacleod
Copy link
Contributor

carmacleod commented Dec 16, 2016 •
edited
Loading

Note that the tree is a keyboard trap in Firefox (Mac and Windows), however this must be a Firefox problem, because tab navigation works fine in both Safari on Mac, and Chrome on Mac and Windows.

@carmacleod
Copy link
Contributor

Ctrl+click on link (which opens link in new tab) should not collapse tree.

@carmacleod
Copy link
Contributor

carmacleod commented Dec 16, 2016 •
edited
Loading

Note that keyboard shortcut Shift+F10 does not bring up a context menu for the treeitem links in Firefox (even though right-clicking on the treeitem link opens a link-related context menu).
Shift+F10 works as I would expect for treeitem links in Chrome (i.e. context menu contains "Open link in new tab", and other link-related menu items).
In Safari, VO+Shift+M opens a context menu on a treeitem link when VoiceOver is running, but it is not the link-related context menu.

I personally prefer the Chrome behavior, because in this example, the tree items really are links. We want the user to know that they are links, and that they can be navigated with enter, or opened in a new tab, or opened in a new Window, or whatever other link-related things are in the context menu.

I agree that we don't want a Screen Reader user to be overwhelmed by all of the (potentially many) treeitem links in the tree. That would make navigation by links, and a List Links dialog almost useless.

@carmacleod
Copy link
Contributor

carmacleod commented Dec 16, 2016 •
edited
Loading

The entries for the Return and Space key behavior say, "Performs the default action (e.g. onclick event) on the current treeitem, in this example updating the "File or Folder Selected" textbox."
However, for this example, the default action for a "branch" treeitem is expand/collapse, and the default action of a "leaf" treeitem is to open the link.

@carmacleod
Copy link
Contributor

Typo: "relies on the browser to computer values"
should be: "relies on the browser to compute values"

@carmacleod
Copy link
Contributor

Typo: "his example implements "
should be: "This example implements "

@carmacleod
Copy link
Contributor

carmacleod commented Dec 16, 2016 •
edited
Loading

For left arrow when focus is on a child node that is also either an end node or a closed node, did you intend to close the parent node after moving focus to it? In other words, the APG says left arrow should move focus to the parent, but the APG does not say to then collapse the parent. It feels a bit unexpected in this example to have the parent collapse when I am just navigating left.

@carmacleod
Copy link
Contributor

Typo in the right arrow function description: "For a treeitem is in the expanded state"
should be: "For a treeitem that is in the expanded state"

@carmacleod
Copy link
Contributor

I find the description of the * (asterisk) behavior somewhat confusing.
It currently says: "Opens all the expandable treeitems in the current leaf (e.g. siblings of the current treeitem)"
Can we change it to: "Opens all expandable treeitems that are siblings of the current treeitem"?

@carmacleod
Copy link
Contributor

carmacleod commented Dec 16, 2016 •
edited
Loading

Typo in the second Usage note for tree role: "Accessible name for the tree widget comes from aria-label attribute."
should say, "Accessible name for the tree widget comes from aria-labelledby="tree1" attribute."

@carmacleod
Copy link
Contributor

Typo in the third Usage note for tree role: "the ul[role"tree"] element "
should say: "the ul[role="tree"] element "

@carmacleod
Copy link
Contributor

Since the tree (ul) is using aria-labelledby instead of aria-label, the Usage note for aria-label="string" should be for aria-labelledby="id". (in this case, the id is "tree1").

@carmacleod
Copy link
Contributor

Typo in the fourth Usage note for group role: "the ul[role"group"] element "
should be: "the ul[role="group"] element "

@carmacleod
Copy link
Contributor

Typo in the first Usage note for treeitem role:
"The role="treeitem" attribute identifies the li element as an ARIA tree widget."
should be:
"The role="treeitem" attribute identifies the li element as an ARIA treeitem widget."

@carmacleod
Copy link
Contributor

Here's a quibble: Is a treeitem really a widget? I think of the tree as being the widget, and the treeitems are just treeitems. So, for example, in the previous comment, I would just say,
"The role="treeitem" attribute identifies the li element as an ARIA treeitem."
If we decide to change this, please note that the phrase "treeitem widget" appears 7 times on this page.

@carmacleod
Copy link
Contributor

carmacleod commented Dec 18, 2016 •
edited
Loading

Just FYI, this example doesn't work well in VoiceOver/Safari or VoiceOver/Chrome, which is probably something that Apple needs to fix. (You may have already opened a radar for this).
Symptoms are:

  • VO thinks the widget is a table containing listboxes (even though the code clearly says role="tree")
  • In Safari, VO doesn't speak the leaves at all (i.e. if you arrow to the anchors, VO says nothing)

@mcking65 mcking65 modified the milestones: Feb 2017 Heartbeat Draft, 1.1 PR Jan 6, 2017
mcking65 added a commit that referenced this issue Feb 11, 2017
@mcking65
Revise Introduction of Navigation Treeview Example Using Declared Pro…
30b4a44 
…perties

Modified examples/treeview/treeview-2/treeview-2b.html for issue #226:
* Corrected typos.
* Editorial revision to wording.
* Added description of function of the example.
* Revised statement that said treeitems cannot contain interactive elements because they can contain other treeitems.
mcking65 added a commit that referenced this issue Feb 13, 2017
@mcking65
Revise features, keyboard, and states/properties for Navigation Treev…
63cf981 
…iew Example Using Declared Properties

For issue #226, modified examples/treeview/treeview-2/treeview-2b.html:
* Under accessibility features, removed redundant information about computed level, setsize, and posinset.
* Under accessibility features, removed incorrect statement: "The expandable <code>treeitem</code> widgets cannot behave as links, their only action can be to open and close a leaf in the tree."
* Added section summarizing treeview terminology with link to the pattern.
* Corrected action defined for enter and space (had the action from the file tree example).
* Revisions for editorial consistency to keyboard table.
* Updated states/properties section header.
* For the tree element, corrected row for label. It specified aria-label instead of aria-labelledby.
* Revisions for editorial consistency to states and properties table.
@annabbott
Copy link

annabbott commented Feb 20, 2017 •
edited
Loading

Deferring to Carolyn's extensive comments.

Plus -
In the Keyboard Support section, currently the following is listed:
Home Moves focus to first node.
End Moves focus to the last node that can be focused without expanding any nodes that are closed.

Question: does Home expand any nodes that are closed?

mcking65 added a commit that referenced this issue Mar 16, 2017
@mcking65
Tree Pattern and Examples: Clarify Home Key Description
3c6c0e2 
For issues #223, #224, #225, and #226, modified the keyboard table of the tree view example pages and the keyboard section of the tree view design pattern.
For the description of the Home key, per suggestion from @annabbott and @MichielBijl, added the phrase "without opening or closing a node".
@mcking65
Copy link
Contributor Author

Two more issues that need to be resolved before we can complete this review:

  1. Issue Navigation Tree View examples: enter key and modifier keys are not correctly handled #364: Navigation Tree View examples: enter key and modifier keys are not correctly handled
  2. Issue Tree View Examples: Fix Incorrect Left Arrow Behavior #365: Tree View Examples: Fix Incorrect Left Arrow Behavior

@mcking65
Copy link
Contributor Author

mcking65 commented Apr 26, 2017 •
edited
Loading

This issue is now dependent on issues #365, #385, and #386.

mcking65 added a commit that referenced this issue May 1, 2017
@mcking65
Treeview Examples: Revise Tree Role Info and Fix Broken JS Source Link
6cb9a58 
For issues #223, #224, #225, and #226, made the following changes.

1. In the `tree` row of the states and properties table, change "an tree" to "a tree".
2. In the `tree` row of the states and properties table, removed unnecessary bullet that defines the tree role.
3. In the `tree` row of the states and properties table, reworded the bullet about the roving tabindex focus management.
4. In the source code section of the navigation treeview examples, removed link to no longer existent treeitemClick.js file.
mcking65 added a commit that referenced this issue Jun 19, 2017
@mcking65
Navigation Treeview Example Using Declared Properties: Fix js link an…
0dd639d 
…d editorial revision

For issue #226, changed examples/treeview/treeview-2/treeview-2b.html:
1. Revised accessibility features section to explain benefit of custom focus and hover styles.
2. Removed link to javascript file that is no longer included.
@mcking65
Copy link
Contributor Author

Since all feedback raised in this issue is addressed, and since there are many comments, closing this issue. Will open a fresh issue to complete the review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
editorial Changes to prose that don't alter intended meaning, e.g., phrasing, grammar. May fix inaccuracies.
Projects
None yet
Milestone
Jan 2017 Clean Up
Development

No branches or pull requests
3 participants
@carmacleod @mcking65 @annabbott