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

moved the aria-live example in page #37520

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dletorey
Copy link
Contributor

@dletorey dletorey commented Jan 6, 2025

Description

  • moved the aria-live example into the page
  • this is a copy of PR #37167 but respects the new page structures

Motivation

  • Helping @bsmth with the migration of examples into content
  • First example moved to make sure I am doing this correctly.

@dletorey dletorey requested a review from a team as a code owner January 6, 2025 13:29
@dletorey dletorey requested review from chrisdavidmills and removed request for a team January 6, 2025 13:29
@github-actions github-actions bot added the size/l [PR only] 501-1000 LoC changed label Jan 6, 2025
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Preview URLs

Flaws (251)

URL: /en-US/docs/Learn_web_development/Core/Accessibility/WAI-ARIA_basics
Title: WAI-ARIA basics
Flaw count: 251

  • macros:
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web/Installing_basic_software
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web/What_will_your_website_look_like
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web/Dealing_with_files
    • Wrong xref macro used (consider changing which macro you use). Error processing path /en-US/docs/Learn/Getting_started_with_the_web/HTML_basics
    • and 246 more flaws omitted
External URLs (1)

URL: /en-US/docs/Learn_web_development/Core/Accessibility/WAI-ARIA_basics
Title: WAI-ARIA basics

(comment last updated: 2025-01-08 12:15:26)

@dletorey dletorey requested review from bsmth and removed request for chrisdavidmills January 6, 2025 13:40
dletorey and others added 3 commits January 6, 2025 14:50
…basics/index.md

Co-authored-by: Brian Smith <brian@smith.berlin>
…basics/index.md

Co-authored-by: Brian Smith <brian@smith.berlin>
</li>
</ul>
<div class="panels">
<article class="active-panel" role="tabpanel" aria-hidden="false">
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond the scope of this PR, but this is a bad practice that happens to be "fine" for this basic example - using aria-hidden=true in this way.

often tab widgets will have other links/focusable elements within them, and if that were the case here, then people could still keyboard tab to those items even when aria-hidden=true is set for the non-active panels. it'd be far more practical / a better example for people to follow if the non-active panels were display none and the active panel display block - negating the need for aria-hidden and allowing one to get rid of the position absolute CSS, which would also likely not be something anyone should do for content like this...

Copy link
Contributor

Choose a reason for hiding this comment

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

looking at the demo in the callback instance of this demo, in the 'hiding things' section of the css+js a11y article, the tab widget demo linked there doesn't behave the same - https://mdn.github.io/learning-area/css/css-layout/practical-positioning-examples/tabbed-info-box.html

it uses display none and uses arrow keys to navigate between the tabs.

it needlessly uses position absolute... but otherwise the demo is far better than what's being used here. pull https://mdn.github.io/learning-area/css/css-layout/practical-positioning-examples/tabbed-info-box.html over here instead of this one

Copy link
Contributor

Choose a reason for hiding this comment

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

related #37524

aria-selected="false"
aria-setsize="3"
aria-posinset="3"
tabindex="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond the scope of this PR, but using aria-posinset ans aria-setsize here is useless. these attributes are only supposed to be used if items might dynamically load into the page. if everything is there by default, they're not supposed to be used.

additionally, a note would be good to add to this section to indicate that while each tab is included in the tab order via tabindex=0, tab widgets are also commonly made where a single tab is in the tab order, and then arrow keys are instead used to navigate between the tabs.

Copy link
Contributor

Choose a reason for hiding this comment

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

if using the demo i linked to in my prior review comment instead, then none of this needs to be addressed since that version of the demo doesn't add the unnecessary ARIA / uses arrow keys to move between the tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scottaohara would this be a better solution for this example?
https://codepen.io/CodeRedDigital/pen/azoEKeo?editors=1010

Copy link
Contributor

@scottaohara scottaohara Jan 7, 2025

Choose a reason for hiding this comment

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

better, but again why not use the version of the demo that's already used on the other MDN page, to be consistent?
That demo also already has the arrow key navigation in place, that is more in line with common tab widget guidance.


<!-- Even is it's not mandatory, it's common practice to put the main navigation menu within the main header -->

<nav role="navigation">
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond scope of PR, but there's no reason to use the navigation landmark here


<!-- A Search form is another common non-linear way to navigate through a website. -->

<form role="search">
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond scope of PR, but this would be a good area to call out that the <search> element could be used to get an implicit role=search landmark.

<!-- Here is our page's main content -->
<main>
<!-- It contains an article -->
<article role="article">
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond scope of PR, there's no reason to use an article role here

</article>

<!-- the aside content can also be nested within the main content -->
<aside role="complementary">
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond scope of PR, but there's no reason to use a complementary role here

@@ -162,7 +361,243 @@ If you go to VoiceOver's landmarks menu (accessed using VoiceOver key + U and th

However, we could do better here. The search form is a really important landmark that people will want to find, but it is not listed in the landmarks menu or treated like a notable landmark beyond the actual input being called out as a search input (`<input type="search">`).

Let's improve it by the use of some ARIA features. First, we'll add some [`role`](/en-US/docs/Web/Accessibility/ARIA/Roles) attributes to our HTML structure. You can try taking a copy of our original files (see [`index.html`](https://github.com/mdn/learning-area/blob/main/accessibility/aria/website-no-roles/index.html) and [`style.css`](https://github.com/mdn/learning-area/blob/main/accessibility/aria/website-no-roles/style.css)), or navigating to our [website-aria-roles](https://github.com/mdn/learning-area/tree/main/accessibility/aria/website-aria-roles) example ([see it live](https://mdn.github.io/learning-area/accessibility/aria/website-aria-roles/)), which has a structure like this:
Let's improve it by the use of some ARIA features.
Copy link
Contributor

Choose a reason for hiding this comment

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

beyond scope of PR, this section / example should be rethought. per my other comments, there's actually very little that was improved here - since aside from the search role, all other roles added were redundant.

it is also sending a confusing message, because the header, footer and man elements did not have roles added to them - so is this trying to imply that the elements that had the roles added somehow lacked support? or were the redundant roles not added to these elements because they were forgotten?

Copy link
Contributor

Choose a reason for hiding this comment

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

made a separate issue for my comments about the unnecessary use of roles in the signposts/landmark section

#37526

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/l [PR only] 501-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants