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

Button Example: Add AT support tables from ARIA-AT #2569

Merged
merged 15 commits into from
Mar 27, 2023
Merged

Conversation

alflennik
Copy link
Contributor

@alflennik alflennik commented Dec 21, 2022

First implementation of #2520

See w3c/aria-at-app#476 for the PR which adds support for ARIA-AT support level iframes.

This PR should not be merged until that branch has been deployed to production, because the iframe included in this PR is still referring to the ARIA-AT sandbox, not production.

Preview Button Example Page


WAI Preview Link failed to build on 'Update site files' step. (Last tried on Mon, 27 Mar 2023 21:42:40 GMT).

@mcking65
Copy link
Contributor

https://deploy-preview-180--aria-practices.netlify.app/aria/apg/patterns/button/examples/button/#supportlevels

gives me

502 Bad Gateway

Under the Support Levels heading.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Suggested minor changes based on issue #2520

content/patterns/button/examples/button.html Outdated Show resolved Hide resolved
content/patterns/button/examples/button.html Outdated Show resolved Hide resolved
@mcking65 mcking65 changed the title Add embed support for ARIA-AT iframes Button Example: Add AT support tables from ARIA-AT Dec 22, 2022
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

We need a table for command button and a table for toggle button.

Now I recall this is why we decided to do the link page before the button page ... we need the JAWS 2023 results before we can publish toggle button. Nevertheless, let's get the toggle results into this PR. We can have a separate PR for the link page.

content/patterns/button/examples/button.html Show resolved Hide resolved
content/patterns/button/examples/button.html Show resolved Hide resolved
content/patterns/button/examples/button.html Show resolved Hide resolved
howard-e added a commit to w3c/aria-at-app that referenced this pull request Jan 11, 2023
howard-e added a commit to w3c/aria-at-app that referenced this pull request Jan 11, 2023
* Address PR feedback shared in w3c/aria-practices#2569

* Account for invalid title being provided for pattern

* style.css update
@howard-e
Copy link
Contributor

@mcking65 the feedback has been addressed and should now be available at https://deploy-preview-180--aria-practices.netlify.app/aria/apg/patterns/button/examples/button/#supportlevels.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

I'm adding this to the agenda for discussion during the January 17 APG TF meeting.

I propose we make the following changes:

  1. Change warning copy as described below.
  2. Make the warning a disclosure like it is on the aria-at site.
  3. Change "None" in table cells to "No Data". The word none implies there is no support. While that is the case for JAWS or NVDA with Safari, it is not the case for VoiceOver with Chrome.
  4. Order rows and columns alphabetically.
  5. Change the accessible names of the buttons:
    1. Change "View Complete Report" to "View Complete Report in New Window"
    2. Change "Embed" to "Copy Embed Link"

Proposed changes to warning copy

The warning is meaningfully different from the production aria-at site and has a gramatical error. Also, reading it in context, I now realize that it is not very clear about the providence of the data. This is particularly problematic if someone uses the embed link in another context.

The copy in this PR preview is:

Candidate Report
The information in this report generated from candidate tests. Candidate aria-at tests are in review by assistive technology developers and lack consensus regarding:

The aria-at pprod site copy copy is:

Warning! Unapproved Report
The information in this report is generated from candidate tests. Candidate ARIA-AT tests are in review by assistive technology developers and lack consensus regarding:

I propose that we:

  1. Use the language "Warning! Unapproved Report" instead of "Candidate Report"
  2. Add the missing word "is" before the word "generated".
  3. Add language that describes the source of the data as the aria-at project with a link to the aria-at home page.

My proposed copy is:

Warning! Unapproved Report
The information in this report is generated from candidate tests developed and run by the ARIA-AT Project. Candidate ARIA-AT tests are in review by assistive technology developers and lack consensus regarding:

@jscholes
Copy link
Contributor

Accessibility feedback:

  1. The iframes don't have accessible names, via a title attribute.
    • Note: when the iframe embed code is copied to the clipboard, the result should also have an accessible name.
  2. The first text within each iframe, e.g. Command Button" and "Candidate Report", are marked up as separate level 3 headings. There should either be a single heading of "Command Button Candidate Report" or similar, or "Candidate Report" shouldn't be a heading at all.
  3. The first column of the table, with the assistive technology names and versions, doesn't have a column header.
  4. The control to navigate to the full report would be more usable/accurate as a link, e.g. which would allow people to easily copy the URL to the clipboard. (in addition to the wording updates already suggested by @mcking65).
  5. The "Embed" button doesn't copy a link to the clipboard, it copies the mark-up for embedding the full iframe. As such, the label should be updated in line with @mcking65's feedback, but not using the word "link". E.g. "Copy embed code".
    • Note: the wording of the resulting alert should also be updated accordingly, to reference embed code rather than a link.

@mfairchild365
Copy link
Contributor

In the ARIA-AT CG call on 1/19, we discussed the wording of the cells. Currently, it says something like "100% of passing assertions". We agreed that it would be clearer to say "100% support" (or whatever the percentage is).

@jscholes
Copy link
Contributor

Additional accessibility note: when reviewing the cells in the first column of the table, the assistive technology name and version are on separate lines of the virtual buffer with NVDA. When reading sequentially, users have to read the name, then arrow down to read the version.

@mcking65
Copy link
Contributor

Feedback from Joe Humbert: the tables have both horizontal and vertical scroll bars when viewed in Firefox but nothing is scrollable b/c all content is visible. Can they be suppressed?

The "None" text does not pass CCR reqs.

The table border colors are not as good contrast as other tables on the page.

Cell text: change to X% supported

@jscholes
Copy link
Contributor

Technical notes:

  1. The embed URL includes a title parameter, e.g. title=Command+Button. Changing the string changes the heading text at the top of the embedded report, which seems like an unnecessary use case that could result in misleading embeds if someone simply calls their embedded report whatever they want.
  2. On a similar note, it would be wonderful for the embed URL to include a parameter for setting the base heading level. It's currently set to 3 because that matches the APG page layout, but that won't be the heading level that's appropriate for all embedded contexts.

@jscholes
Copy link
Contributor

Final comment (I think!): I ran the page through Axe, and it noted that the same element ID, at-version, is being used on several elements. I don't think this has a user-perceivable impact, but valid mark-up is just a good idea.

@mcking65
Copy link
Contributor

@jscholes can you explain the advantage or benefit of adding a title attribute on the iframe? Generally, I think it is better if iframes do not have an accessible name. To me, it seems like it is a much better experience if the screen reader experience is more like the visual experience where the iframe boundaries are not distracting.

During the APG meeting, we discussed link verses button for the view full report element. While there are some marginal advantages to a link with an href, there are also some visual presentation considerations. We don't want to style a link like a button nor a button like a link. The copy embed code element needs to be a button. Several members of the task force prefer the current design with two buttons over exploring a design with a link and a button. Since the benefits of the change are so minimal, I would prefer to keep the focus on all the highest priority changes. We can revisit this later.

@mcking65
Copy link
Contributor

mcking65 commented Jan 20, 2023

@howard-e @alflennik

We collected a fair bit of feedback from both the APG TF meeting and the ARIA-AT CG meeting this week. Most of it is represented in the comments above. However, to ensure we have a clear and prioritized list of changes for you to work on, I'm consolidating, slightly revising, and prioritizing all the feedback in this comment.

Requested Changes

  1. Increase the color contrast ratio of the text in the cells that contain the word "None" to the required 4.5 or better.
  2. Change copy in cells with data to "%X Supported", e.g. "96% Supported".
  3. In cells that contain the word "None", change copy to one of the two options described below.
  4. Change copy of the text above the table (warning text) as described below.
  5. Remove heading markup from the warning and make it a disclosure that is collapsed on load like it is on the aria-at site.
  6. Change the accessible name of the button "View Complete Report" to "View Complete Report in New Window".
  7. Change the visible name of the button "Embed" to "Copy Embed Code" and revise the confirmation announcement to be "Copied code for embedding".
  8. Either by using a visible caption or by using aria-label, give the table an accessible name of "Assistive Technology Support for PATTERN_NAME", e.g., "Assistive Technology Support for Command Button". Don't allow callers to change the caption/name.
  9. Do not include a heading for the content, e.g., the h3 with text "Command Button", in the iframe. The iframe should contain only the warning/providence disclosure, the table, and the buttons for full report and embedding. On pages like the button example page where there are multiple examples, we will include the H3 markup in the example page content followed by the iframe.
  10. Order rows and columns in the table alphabetically.
  11. Increase the color contrast of the table borders (make them match other tables on the page).
  12. If possible, suppress the visibility of the table scrollbars when they are not needed.

New Warning/Providence disclosure copy

The text above the table is meaningfully different from the production aria-at site and has a gramatical error. Also, reading it in context, I now realize that it is not very clear about the providence of the data. This is particularly problematic if someone uses the embed link in another context.

The copy in this PR preview is:

Candidate Report
The information in this report generated from candidate tests. Candidate aria-at tests are in review by assistive technology developers and lack consensus regarding: ...

The aria-at pprod site copy copy is:

Warning! Unapproved Report
The information in this report is generated from candidate tests. Candidate ARIA-AT tests are in review by assistive technology developers and lack consensus regarding: ...

Requested changes:

  1. Use the language "Warning! Unapproved Report" instead of "Candidate Report"
  2. Add the missing word "is" before the word "generated".
  3. Add language that describes the source of the data as the aria-at project with a link to the aria-at home page.

Requested new copy:

Warning! Unapproved Report
The information in this report is generated from candidate tests developed and run by the ARIA-AT Project. Candidate ARIA-AT tests are in review by assistive technology developers and lack consensus regarding:

  1. applicability and validity of the tests, and
  2. accuracy of test results.

Note: as requested above, "Warning! Unapproved Report" should not be a heading. Instead, it should be summary text in a summary/details or otherwise made into a disclosure button label.

Options for cells that do not contain data

We have two reasons for a cell to not have data:

  1. We have not yet tested the AT/Browser combination, e.g., JAWS/Firefox.
  2. The AT/Browser combination does not exist, e.g., JAWS/Safari.

If it is readily feasible to have a different string for each case, please use the following Option A for the copy, otherwise use Option B.

Option A for the copy in cells with no data is:

  1. If the AT/Browser combination will be tested in the future, use the string "Data Not Yet Available"
  2. If the AT/Browser combination does not exist, use the string "Not Applicable".

Option B for the copy in cells with no data is "No Data".

@jscholes
Copy link
Contributor

@mcking65

can you explain the advantage or benefit of adding a title attribute on the iframe?

Iframes are user-addressable containers that are conveyed by the screen reader, and can be directly navigated to. In the case of this preview page, there are also two of them, without any means of telling them apart without reading the content.

Hence, in line with https://www.w3.org/WAI/WCAG21/Techniques/html/H64, I would currently consider these frames to be failing against 4.1.2 Name, Role, Value (level A) were I auditing the page.

@mcking65
Copy link
Contributor

mcking65 commented Jan 21, 2023

@jscholes wrote:

Iframes are user-addressable containers that are conveyed by the screen reader, and can be directly navigated to. In the case of this preview page, there are also two of them, without any means of telling them apart without reading the content.

Hence, in line with https://www.w3.org/WAI/WCAG21/Techniques/html/H64, I would currently consider these frames to be failing against 4.1.2 Name, Role, Value (level A) were I auditing the page.

James, I appreciate your attention to detail and the pointer to h64. However, I still don't follow your reasoning. This might be one of those cases where there are several different and reasonable readings of WCAG.

My reading of 4.1.2 and H64 lead me to a different conclusion. I wonder what you think of the following line of reasoning?

From the applicability section of H64: Using the title attribute of the iframe element | WAI | W3C:

This technique relates to:

  • 2.4.1: Bypass Blocks (Sufficient, together with H70: Using frame elements to group blocks of repeated material when used with Grouping blocks of repeated material in a way that can be skipped, using one of the following techniques: )
  • 4.1.2: Name, Role, Value (Sufficient when used with G108: Using markup features to expose the name and role, allow user-settable properties to be directly set, and provide notification of changes)

In my reading of the H64 explainer, the APG example pages are not using frames in a way where the technique is applicable. They are not using frames to group blocks or structure the page content. For all practical purposes, the iframes are exactly equal in function and semantics to the section and div elements that wrap other content on the page. We do not intent the frames to have any accessibility semantics.

The default role of an iframe is presentation in most browsers. Giving the iframe a name overrides role presentation. The iframe element is similar to the section element in this way. The section element has role generic by default. However, if you name a section, it becomes a landmark region element.

We have not named the section elements because we don't want to overload the page with landmark regions; we have a well-defined document information architecture inside the main landmark region that is conveyed by headings. If we were to name the sections, it would reduce users ability to perceive the overall architecture of the page outside the main region and efficiently navigate to any part of the page.

Similarly, I believe we want the iframes to be presentational because they fit into that same document information architecture within the main content, which is communicated via headings. Naming the iframes would disrupt the information architecture without adding navigation value. This will be especially the case when we move the H3 elements outside the iframes.

Note that I've requested that the table be named. The consumer of the iframe, especially the APG pages, should be in full control of the heading structure and content. On pages that have only one example, we do not want an H3 heading. We just want an appropriately named table. This will match the structure of the rest of the page.

@alflennik
Copy link
Contributor Author

@mcking65 A preview including fixes for the above items is available here: https://deploy-preview-206--aria-practices.netlify.app/

Note that item 3 has not been fixed - we discussed it previously at an APG meeting and decided that if it was too heavy a lift it would be preferable to put it aside and implement the other fixes first.

@mcking65
Copy link
Contributor

mcking65 commented Mar 9, 2023

Note that item 3 has not been fixed - we discussed it previously at an APG meeting and decided that if it was too heavy a lift it would be preferable to put it aside and implement the other fixes first.

We have two options for item 3: option A and option B. We agreed on option B if option A is too heavy a lift for now.

Per option B described above, The copy should be "No Data" instead of "None".

Also, the warning text is missing some words. Please double check against the copy listed above. The phrase before the list is missing the word "Candidate".

@alflennik
Copy link
Contributor Author

@mcking65 I saw you added headers for each of the iframes, thank you for fixing that. Also I think you were asking about why the preview didn't update automatically: the reason is that this feature requires changes across both the aria-practices and wai-aria-practices repositories, and the automated preview system is intended for changes to aria-practices which don't require changes in wai-aria-practices.

The issues you mentioned have been fixed, take a look here: https://deploy-preview-206--aria-practices.netlify.app/aria/apg/patterns/button/examples/button/

Additionally, following our conversation in the task force meeting last week I added support for recommended reports in addition to candidate reports.

@mcking65 mcking65 requested review from charmarkk and jongund March 21, 2023 18:20
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Review updates to first AT support table - pull request #2569.

The full IRC log of that discussion <jugglinmike> Subtopic: Review updates to first AT support table - pull request #2569
<jugglinmike> github: https://github.com//pull/2569
<jugglinmike> AlexFlenniken: the last thing I changed was about a week ago--changing the default height
<jugglinmike> Matt_King: I shared some feedback--was that addressed?
<jugglinmike> AlexFlenniken: Yes, I've addressed that
<jugglinmike> Matt_King: Then this is ready for everybody to look at
<jugglinmike> Matt_King: It looks like Howard_Edwards (not present) and myself are assigned as reviewers. I would like to have some other members of the task force looking at this
<jugglinmike> jongund: I can take a look at it
<jugglinmike> MarkMcCarthy: You can add me, too
<jugglinmike> Matt_King: The long-term plan is that there will only be one example per page. We have a backlog item to address the patterns which have more than one example per page (e.g. button)
<jugglinmike> jongund: Do you want me to work on separating the button examples into separate pages?
<jugglinmike> Matt_King: Thank you, but not right now. I think that at the current time, doing so would create too much churn

@mcking65
Copy link
Contributor

@alflennik

The two issues I raise in this comment above are not addressed.

  1. In the tables, it still says "none" instead of "no Data"
  2. Above the table, when the disclosure is expanded, the copy says "ARIA-AT tests are in review by assistive technology developers and lack consensus regarding:". It is missing the word "Candidate". It should say "Candidate ARIA-AT tests are in review by assistive technology developers and lack consensus regarding:"

Neither of the above issues is visible at this moment because the iframe is returning:

Error: ENOENT: no such file or directory, open '/home/aria-bot/aria-at-report/handlebars/views/main.hbs'

Copy link
Contributor

@charmarkk charmarkk left a comment

Choose a reason for hiding this comment

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

  • There are some wrapping issues with the first bullet in the "About This Example" section. Shrinking the screen eventually fixes it, until the mobile breakpoint is triggered and it happens again. The bullet is slightly overlapping the button icon.

image

  • There is no visual focus indicator on the example Print Page and Mute buttons - should there be?

  • The text wrapping/breaks in the HTML Source Code section is really odd and hard to visually parse. It looks like it's similar to most other examples, but because there are lots of single words and elements not strictly related to ARIA (notably <q> and <code>) it it is a little hard to read it. Should that be looked at?
    image

@alflennik
Copy link
Contributor Author

@charmarkk thank you for the review! I think you found a couple of genuine issues that are worth addressing across the whole site, not just the button page.

Since these changes would be broad, and this PR is focused on just this one page, I want to keep this PR focused on the iframes and deal with those issues separately.

Would you be willing to open two issues? Also if you could add alt-text to the screenshots that would probably help get it addressed faster. Thank you for your help!

@charmarkk
Copy link
Contributor

@alflennik fair! Can do.

With respect to the tables, I can +1 @mcking65's previous comments, too.

@alflennik
Copy link
Contributor Author

@mcking65 I believe the issues you saw have been fixed, could you check once more? Thank you!

@alflennik
Copy link
Contributor Author

@mcking65 I've updated this PR and the website PR to use the production version of ARIA-AT following our deploy of that feature today. It can be previewed here: https://deploy-preview-206--aria-practices.netlify.app/aria/apg/patterns/button/examples/button/

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Review updates to first AT support table - pull request #2569.

The full IRC log of that discussion <jugglinmike> Subtopic: Review updates to first AT support table - pull request #2569
<jugglinmike> github: https://github.com//pull/2569
<jugglinmike> Matt_King: Victory! AT support tables for Button and Toggle Button are merged
<jugglinmike> Matt_King: Unfortunately, they aren't visible in the preview of the "main"branch
<jugglinmike> Alex_Flenniken: The link that you shared with me is an outdated deployment
<jugglinmike> Alex_Flenniken: here's the link to the active deployment https://aria-practices.netlify.app/aria/apg/patterns/button/examples/button/
<jugglinmike> Matt_King: Ah, there it is. This is good news!
<jugglinmike> Matt_King: So our plan is that we will ask Shawn to do the publication to production on April 11
<jugglinmike> Matt_King: In the mean time, there's a lot that needs to go on to prepare for that launch
<jugglinmike> Matt_King: I'm having meetings with Apple and Vispero, preparing a blog post (hopefully getting a quote from Vispero and/or Apple), add support tables to the link, Alert, and Radio Button with Active Descendent example pages (I'll make a pull request for that soon)
<jugglinmike> Matt_King: Once we have all that lined up and receive a "Green light" from all of our stakeholders, we'll proceed with a **launch** on April 11 and and an **announcement** on April 13
<jugglinmike> Matt_King: I don't think we'll be deploying to production between then and now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants