Address support tables 2.0 feedback #521

alflennik · 2023-03-13T18:30:31Z

Implements fixes for additional issues found here: w3c/aria-practices#2569

aleenaloves · 2023-03-13T20:22:35Z

Summary

Pending explanations/implementations/corrections on items I was unable to verify this is good to approve :)

General Code Review

Generally, everything looks awesome @alflennik ! I only had the following question, but it doesn't bar this PR from being approved.

I know in CSS we can also use variables/custom properties, in terms of some of the feedback where we get told to use the same colors as xyz, would this be a helpful pattern to start implementing in a separate PR?

In reference to the original PR

Increase the color contrast ratio of the text in the cells that contain the word "None" to the required 4.5 or better. ✅
Change copy in cells with data to "%X Supported", e.g. "96% Supported". ✅
In cells that contain the word "None", change copy to one of the two options described below. ✅

Partially Complete

Change copy of the text above the table (warning text) as described below. ✅
Remove heading markup from the warning and make it a disclosure that is collapsed on load like it is on the aria-at site. ✅
Change the accessible name of the button "View Complete Report" to "View Complete Report in New Window". ✅
Change the visible name of the button "Embed" to "Copy Embed Code" and revise the confirmation announcement to be "Copied code for embedding". ✅
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.

❌❔ the preview currently reads Assistive Technology Support for Command Button **Example**

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.

❔ unable to verify, could not find an example of a single one in the preview

Order rows and columns in the table alphabetically. ✅
Increase the color contrast of the table borders (make them match other tables on the page).

❌ Contrast Value

If possible, suppress the visibility of the table scrollbars when they are not needed. ✅

howard-e

Looks good to me @alflennik! While I do have additional questions listed below, those to me aren't blocking as they can be updated once we receive further feedback and directives there.

What I do think is blocking (which this PR wasn't intended to touch or address) is around how the CANDIDATE/RECOMMENDED status is evaluated. server/apps/embed.js lines 92:94 is being done against all reports which have existed and would have previously had a CANDIDATE/RECOMMENDED status. This can quite easily prevent the correct disclaimer being shown above the table. This evaluation should be done further down against latestReports so it is actually done against the data being shown in the table.

howard-e · 2023-03-13T20:09:45Z

server/handlebars/views/main.hbs

                <ol>
                    <li>applicability and validity of the tests, and</li>
                    <li>accuracy of test results.</li>
                </ol>
            </div>
        </details>
        {{else}}
-        <div id="embed-report-status-container">
-            <div id="candidate-title" class="recommended"><h3>Recommended Report</h3></div>
+        <details id="embed-report-status-container">


Thanks for updating the markup to allow the RECOMMENDED disclaimer to be a part of the updated disclosure as well!

howard-e · 2023-03-13T20:14:00Z

server/handlebars/views/main.hbs

-        <div id="embed-report-status-container">
-            <div id="candidate-title" class="recommended"><h3>Recommended Report</h3></div>
+        <details id="embed-report-status-container">
+            <summary id="candidate-title" class="recommended"><span>Recommended Report</span></summary>


I'd be curious about what the community's thoughts would on this title. Should the disclaimer also be the reverse of the CANDIDATE disclaimer, so Approved Report?

This is also done on the application's Reports pages as well.

It would be good to have them review it. Can we promote one of the reports to a recommended state on the sandbox?

server/handlebars/public/style.css

alflennik · 2023-03-13T20:59:31Z

@aleenaloves First of all thanks for the detailed review!

alflennik · 2023-03-13T21:25:10Z

@aleenaloves About "Increase the color contrast of the table borders," you noted that the contrast value is short of the recommended 3:1 value. It seems like this is an issue for the entire site, since we're reusing the same value from the entire site. How do you think we should fix it?

alflennik · 2023-03-13T21:27:39Z

@aleenaloves Thank you for noticing that the test titles have inconsistent names (some have the word Example and some don't). While we could definitely work around that within the embed system I think it makes sense to take this as an opportunity to improve the overall consistency of our data, and I'll bring this up with the rest of the team.

alflennik · 2023-03-13T21:30:56Z

@howard-e Thanks for finding that issue with the candidate / recommended algorithm!

aleenaloves · 2023-03-13T21:33:49Z

@aleenaloves About "Increase the color contrast of the table borders," you noted that the contrast value is short of the recommended 3:1 value. It seems like this is an issue for the entire site, since we're reusing the same value from the entire site. How do you think we should fix it?

You're right that this is an issue for the entire website. Since this would then be a design concern, it may make sense to get isaac's input on it - but irregardless, irrelevant to this PR in particular.

Since the 2nd part of matt's original step 11 also included:

(make them match other tables on the page)

Not sure what tables he's exactly referring to, but if it's the Role, Property, State, and Tabindex Attributes table, then that uses a border color of silver or #C0C0C0.

But i defer to you on what you think he meant/what you think would be best in this scenario.

alflennik · 2023-03-14T20:21:41Z

I did make an issue about the contrast here, w3c/wcag#3102, but for now it seems like the best move is to keep the contrast consistent with the other tables on the site which was the ask originally.

aleenaloves · 2023-03-14T20:29:32Z

thanks @alflennik ! loved the investigation ~

i'm good to approve this, pending @howard-e's notes :D

alflennik · 2023-03-15T16:00:52Z

@howard-e @aleenaloves So I reproduced and fixed the issue Howard found, but I discovered another issue in the process - namely that the caching solution in use right now has no invalidation built in, so it could potentially get pretty far out of date and would potentially only be refreshed when deploys happen. I'm currently working on a solution to that.

alflennik · 2023-03-20T15:12:54Z

I talked to a couple of folks at WCAG and buttons and tables are not subject to the 3:1 requirement - seems like a good bit of knowledge for the toolbox for next time 😄

evmiguel

LGTM! Great job!

Address support tables 2.0 feedback

99813f7

alflennik requested review from howard-e and aleenaloves March 13, 2023 18:30

howard-e reviewed Mar 13, 2023

View reviewed changes

Fix candidate recommended algorithm

3b9b688

Add additional caching

a7056e6

alflennik added 8 commits March 20, 2023 21:05

Add test

3b2f725

Resolve circular dependency issue

9dca1c5

Support slower environments in test

07c2197

Implement basic test

b605249

Finish tests

0c3aea9

Support even slower environments

0066529

Fix non local environments

d6b4f4c

Fix test environment

6699a39

evmiguel approved these changes Mar 27, 2023

View reviewed changes

howard-e approved these changes Mar 27, 2023

View reviewed changes

alflennik merged commit e12e32b into main Mar 27, 2023

alflennik deleted the support-tables-2.0-feedback branch March 27, 2023 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address support tables 2.0 feedback #521

Address support tables 2.0 feedback #521

alflennik commented Mar 13, 2023

aleenaloves commented Mar 13, 2023

howard-e left a comment •

edited

Loading

howard-e Mar 13, 2023

howard-e Mar 13, 2023

alflennik Mar 13, 2023

alflennik commented Mar 13, 2023

alflennik commented Mar 13, 2023

alflennik commented Mar 13, 2023

alflennik commented Mar 13, 2023

aleenaloves commented Mar 13, 2023

alflennik commented Mar 14, 2023

aleenaloves commented Mar 14, 2023

alflennik commented Mar 15, 2023

alflennik commented Mar 20, 2023

evmiguel left a comment

Address support tables 2.0 feedback #521

Address support tables 2.0 feedback #521

Conversation

alflennik commented Mar 13, 2023

aleenaloves commented Mar 13, 2023

Summary

General Code Review

In reference to the original PR

howard-e left a comment • edited Loading

Choose a reason for hiding this comment

howard-e Mar 13, 2023

Choose a reason for hiding this comment

howard-e Mar 13, 2023

Choose a reason for hiding this comment

alflennik Mar 13, 2023

Choose a reason for hiding this comment

alflennik commented Mar 13, 2023

alflennik commented Mar 13, 2023

alflennik commented Mar 13, 2023

alflennik commented Mar 13, 2023

aleenaloves commented Mar 13, 2023

alflennik commented Mar 14, 2023

aleenaloves commented Mar 14, 2023

alflennik commented Mar 15, 2023

alflennik commented Mar 20, 2023

evmiguel left a comment

Choose a reason for hiding this comment

howard-e left a comment •

edited

Loading