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.

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

Navigation Treeview Example: Convert to single page example with functionality equivalent to disclosure navigation example #1558

Merged
merged 81 commits into from
Jan 19, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Oct 7, 2020

Resolves #1526 by revising the navigation tree view example.

Changes:

  • Deleted previous treeview for links examples
  • Created a new file treeview-navigation.html

To dos:

  • Change images for collapsing and expanding treeitems to use SVG
  • Regression tests still need to be completed.

Preview link

View revised example in compare branch

Review checklist


Preview | Diff

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2020

Regression test coverage:

Examples without any regression tests:

  • dialog-modal/alertdialog.html

Examples missing some regression tests:

  • dialog-modal/datepicker-dialog.html:
    • textbox-aria-describedby
  • menu-button/menu-button-actions-active-descendant.html:
    • menu-up-arrow
    • menu-down-arrow
    • menu-character
  • toolbar/toolbar.html:
    • toolbar-tab
    • toolbar-right-arrow
    • toolbar-left-arrow
    • toolbar-home
    • toolbar-end
    • toolbar-toggle-esc
    • toolbar-toggle-enter-or-space
    • toolbar-radio-enter-or-space
    • toolbar-radio-down-arrow
    • toolbar-radio-up-arrow
    • toolbar-button-enter-or-space
    • toolbar-menubutton-enter-or-space-or-down-or-up
    • toolbar-menu-enter-or-space
    • toolbar-menu-down-arrow
    • toolbar-menu-up-arrow
    • toolbar-menu-escape
    • toolbar-spinbutton-down-arrow
    • toolbar-spinbutton-up-arrow
    • toolbar-spinbutton-page-down
    • toolbar-spinbutton-page-up
    • toolbar-checkbox-space
    • toolbar-link-enter-or-space
    • toolbar-spinbutton-role

Example pages with Keyboard or Attribute table rows that do not have data-test-ids:

  • dialog-modal/alertdialog.html
    • "Keyboard Support" table(s):
      • Tab
      • Shift + Tab
      • Escape
      • Command + S
      • Control + S
    • "Attributes" table(s):
      • alertdialog
      • aria-labelledby=IDREF
      • aria-describedby=IDREF
      • aria-modal=true
      • alert

SUMMARY:

54 example pages found.
1 example pages have no regression tests.
3 example pages are missing approximately 27 out of approximately 769 tests.

ERROR - missing tests:

Please write missing tests for this report to pass.

@jongund
Copy link
Contributor Author

jongund commented Oct 7, 2020

@spectranaut
What does Travis CI failing mean?
Is this something I should worry about?

@nschonni
Copy link
Contributor

nschonni commented Oct 7, 2020

@jongund it's flagging some HTML validator issues

"file:/home/travis/build/w3c/aria-practices/examples/treeview/treeview-navigation.html":223.13-223.85: error: The “main” element must not appear as a descendant of the “section” element.

"file:/home/travis/build/w3c/aria-practices/examples/treeview/treeview-navigation.html":223.13-223.85: error: The “main” element must not appear as a descendant of the “main” element.

"file:/home/travis/build/w3c/aria-practices/examples/treeview/treeview-navigation.html":223.13-223.85: error: A document must not include more than one visible “main” element.

"file:/home/travis/build/w3c/aria-practices/examples/treeview/treeview-navigation.html":508.62-508.69: error: Stray end tag “group”.

"file:/home/travis/build/w3c/aria-practices/examples/treeview/treeview-navigation.html":508.136-508.140: error: End tag “td” seen, but there were open elements.

"file:/home/travis/build/w3c/aria-practices/examples/treeview/treeview-navigation.html":508.51-508.56: error: Unclosed element “code”.

@mcking65
Copy link
Contributor

@jongund, thank you, this is wonderful progress!

Do we need that much filler content? It takes a long time to read through it. Can we have short, simple content that matches what we have in the
Example Disclosure for Navigation Menus

Instead of a complementary landmark region for the filler, can we use a region named "Mythical University sample page content"? That is what we have on the disclosure example page.

In the disclosure example, activating a link moves focus to the heading of the content. Shouldn't we do the same here?

@mcking65
Copy link
Contributor

When the page loads, the home content page is displayed, but it doesn't have aria-current set.

@mcking65
Copy link
Contributor

I expanded about and loaded overview. If I refresh the page, the about branch is collapsed so the user can't see which page is current.

Should the current page always receive focus when tabbing into the tree?

After expanding a parent tree item, the name seems to get duplicated. For example, JAWS reads about as "Open about about".

@carmacleod carmacleod self-requested a review October 13, 2020 18:57
@a11ydoer a11ydoer self-requested a review October 13, 2020 18:57
@jongund
Copy link
Contributor Author

jongund commented Oct 15, 2020

@mcking65
Made some updates to reduce filler content and set tabindex=0 on the current page.

@mcking65
Copy link
Contributor

@jongund commented:

I removed the section about default borders around treeitems, we only need default borders around buttons.

Thank you!

I also added a note in the "Focus Movement After Content Load" section when keyboard focus is moved:
Note: Keyboard only users will need to navigate back to the navigation treeview to activate other items to view changes in content. To make navigation back to the treeview efficient, the treeview should be one tab stop away from the element with focus.

Excellent addition. Thank you.

I revised a bit to remove the word "should", reduce words, and to emphasize the need for the design to accommodate the tab sequence. We wouldn't want people to make a visually unpredictable tab sequence to make it 1 tab stop away.

@a11ydoer
Copy link
Contributor

a11ydoer commented Dec 15, 2020

Communicating with @jongund regarding color contrast issue. - tree item links in hover status.

Foreground color, #005a9c and Background color #ccc has color contrast ratio of 4.44:1. This fails Nomal text size WCAG AA, WCAG AAA, Large text WCAG AAA.

Here is the CSS code.
.treeview-navigation a[role="treeitem"]:hover {
background-color: #ccc;
text-decoration: underline;
padding-left: 4px;
border-left: 5px solid #333;
}

.treeview-navigation a[role="treeitem"] {
margin: 0;
padding: 4px;
padding-left: 9px;
text-decoration: none;
color: #005a9c;
border: none;
display: block;
}

Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

Tree item link hover color contrast issue can be addressed before the merge.

@a11ydoer a11ydoer requested a review from jnurthen December 15, 2020 19:47
@jongund
Copy link
Contributor Author

jongund commented Dec 15, 2020

@a11ydoer
Updated hover color

Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

Changed background color, #ADDDFF now has color contrast ratio of 4.94:1 and it pass Normal text WCAG AA, Large text WCAG AA and WCAG AAA.

@a11ydoer
Copy link
Contributor

@mcking65 and @jnurthen
Would you two mind reviewing this PR so that we can merge this? Once you are done, please approve PR or check your name in review check list.

@jongund
Copy link
Contributor Author

jongund commented Jan 12, 2021

@mcking65
Fixed bug with aria-current link being visible when treeview loses focus and added test case to regression tests. So this should be ready to merge.

* Confirm the aria-owns element.
*
* @param {obj} t - ava execution object
* @param {String} elementSelector - the element with aria-owns set
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param {String} elementSelector - the element with aria-owns set
* @param {string} elementSelector - the element with aria-owns set

/**
* ARIA Treeview example
* @function onload
* @desc after page has loaded initialize all treeitems based on the role=treeitem
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @desc after page has loaded initialize all treeitems based on the role=treeitem
* @description after page has loaded initialize all treeitems based on the role=treeitem

this.fillerTextSentences.push(
'The content on this page is associated with the <a href="$linkURL">$linkName</a> link for <a href="$siteURL">$siteName</a>.'
);
// this.fillerTextSentences.push('The text content in this paragraph is filler text providing a detectable change of content when the <a href="$linkURL">$linkName</a> link is selected from the menu. ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete commented out code

}

/**
* ARIA Treeview example
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ARIA Treeview example
* ARIA Treeview example
*

@mcking65
Copy link
Contributor

@jongund
Thank you for all your work on this. This is a huge step forward!
We have all required reviews complete, all tests are passing, and this is now ready to ship!

@mcking65 mcking65 merged commit f943b4d into master Jan 19, 2021
@mcking65 mcking65 deleted the update-tree-nav branch January 19, 2021 15:42
@carmacleod
Copy link
Contributor

@nschonni
It seems that your suggestions and Matt's review crossed paths within a minute of each other somewhere on the internet, so your suggestions weren't merged.
Maybe create a new little PR for your suggestions?

@nschonni
Copy link
Contributor

Thanks @carmacleod I spun up #1728

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise Navigation Tree Example to match Disclosure Navigation Example
9 participants