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

Coverage and Quality Report: Add reporting on use of forced-colors media query and currentcolor value #3024

Merged
merged 9 commits into from
Jul 23, 2024

Conversation

jongund
Copy link
Contributor

@jongund jongund commented May 20, 2024

The pull request updates the coverage script to:

  1. Remove information about the use of forced-color-adjust from the report, since I thin we should not even be using it anymore in examples.
  2. Adds information on examples using forced-colors media query.
  3. Adds information on examples using currentcolor value for CSS properties that support color values.

Preview of Updated Coverage Report


WAI Preview Link (Last built on Tue, 23 Jul 2024 06:01:44 GMT).

@mcking65
Copy link
Contributor

@jongund is this a replacement for #2902

@jongund
Copy link
Contributor Author

jongund commented Jun 3, 2024

@mcking65
Yes this would be the report we would want now.
Writing the high contrast support practices clarified for me the relevant information we needed in the report.

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Quality report tracking of force-colors.

The full IRC log of that discussion <Jem> Topic:Quality report tracking of force-colors
<Jem> https://github.com//pull/3024
<Jem> github:https://github.com//pull/3024
<dmontalvo> Matt_King: Is this removing information for force-colors?
<Jem> preview page: https://deploy-preview-324--aria-practices.netlify.app/aria/apg/about/coverage-and-quality/
<dmontalvo> jongund: Just for SVG. I think in HTML force-color adjust is set
<dmontalvo> ... I remember that for SVG elements we had to add this because the default was set to none, at least that's how the browsers dealt with it when we started
<dmontalvo> Matt_King: Do you still think we shouldn't tracking it?
<dmontalvo> jongund: We should be tracking force-colors media query and current color
<dmontalvo> ... Force-colors won't work if force-color adjust is set to none
<dmontalvo> ... I think that's something we are added to SVG because we are not ssure. I don't think it's something we need to track
<dmontalvo> Matt_King: We are tracking the things that we want. We don't have to worry if force-color media query is used appropriately and we test it in high contrast mode
<dmontalvo> ... We're going to need some reviewers
<dmontalvo> Matt_King: It seems there is an empty file "aria-practices" created in your PRs
<dmontalvo> Jem: I don't see that file
<dmontalvo> jongund: I fixed it
<dmontalvo> Matt_King: I think we need someone to review the JavaScript to ensure this is doing what it should
<dmontalvo> Jem: This looks good, not sure how I can test this
<dmontalvo> Matt_King: One way would be to do some searching of the code base
<dmontalvo> jongund: In the current repo it does look at the example files, it does not matter whether it is SVG
<dmontalvo> ... If it's a CSS file it loads it as text and looks through it
<dmontalvo> Matt_King: There were only two examples when I looked at the preview
<dmontalvo> ... According to this table we have exactly 50 examples
<Jem> https://deploy-preview-324--aria-practices.netlify.app/aria/apg/about/coverage-and-quality/
<dmontalvo> Matt_King: Why are we tracking the CSS before and after in these columns?
<Jem> we are looking at "graphics techniques" table
<dmontalvo> jongund: Using the actual attribute was considered a best practice
<dmontalvo> Matt_King: Mostly we don't talk about engineering as a practices
<dmontalvo> jongund: Purpose is to identify examples that are using them in particular
<dmontalvo> Matt_King: The aim is to look for places where we are missing something
<dmontalvo> ... Where we are not doing it and should be doing it
<dmontalvo> ... Maybe we need something like "missing", "needed", or the like
<dmontalvo> jongund: Something like an aria-checked that is not using this technique?
<dmontalvo> Matt_King: There is a certain technique that we need to make sure we use in each of the example if we are saying so
<dmontalvo> ... If there are other legitimate ways to achieve the same thing, maybe we don't need it in the report
<dmontalvo> Jem: CSS content information is important because it's related to the accessible name. I don't remember exatly how it's impacting accessibility
<dmontalvo> jongund: If there's content it will be added to the accessible name
<dmontalvo> ... But if the content is an image gotten from a URL then it gets ignored
<dmontalvo> Matt_King: In the first example the content is blank, and in the other we have yes. It may make people wonder what this means
<dmontalvo> ... This is out of scope for this agenda item, but we need to thing about approaches to check the quality of our code
<dmontalvo> jongund: Not sure why we need to track those in particular
<dmontalvo> ... I guess I could look for all the examples that use aria-checked, maybe aria-selected
<dmontalvo> Matt_King: What about aria-pressed?
<dmontalvo> jongund: Yes.
<dmontalvo> Matt_King: We may want a column that is something like "state not triggered by CSS" or something
<dmontalvo> jongund: I don't know if you can say you have to do it that way
<dmontalvo> ... Whatever state you are susing an ARIA prperty, you should control the visual rendering
<dmontalvo> ... It's not a requirement
<dmontalvo> Matt_King: If we don't have a requirement to code it a specific way, then we probably don't need it in this report
<dmontalvo> ... If we want to track consistency, maybe we do need it in the report
<dmontalvo> Matt_King: Can you crete a new issue with the discussion we just had?
<dmontalvo> s/Matt_King: /Jem: Matt_King, /
<dmontalvo> Jem: I could create it an assign it to you Matt_King

@mcking65 mcking65 requested a review from howard-e June 11, 2024 18:13
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

@jongund changes look good! Other than the questions in line, could you also merge this with main so #3026 changes could be included?

npm run coverage-report can be ran again afterwards to make sure things are in sync.

@@ -521,7 +521,9 @@ glob
contentCSS: getNumberOfReferences(dataCSS, 'content'),
beforeCSS: getNumberOfReferences(dataCSS, '::before'),
afterCSS: getNumberOfReferences(dataCSS, '::after'),
forcedColorAdjust: getNumberOfReferences(dataCSS, 'forced-color-adjust'),
// forcedColorAdjust: getNumberOfReferences(dataCSS, 'forced-color-adjust'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be removed instead of commenting out?

scripts/coverage-report.js Outdated Show resolved Hide resolved
$('#example_summary_force_color').html(countForcedColorAdjust);
$('#example_summary_force_colors').html(countForcedColors);
$('#example_summary_current_color').html(countCurrentColor);
// $('#example_summary_force_color_adjust').html(countForcedColorAdjust);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

Comment on lines +178 to +183
<!--
<tr>
<th scope="row">Uses <code>forced-color-adjust</code> in CSS/SVG</th>
<td id="example_summary_force_color_adjust"></td>
</tr>
-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take it out

@mcking65 mcking65 changed the title Update/coverage to add forced-colors media query and currentcolor value to coverage report Coverage and Quality Report: dd forced-colors media query and currentcolor value to coverage report Jun 18, 2024
@mcking65 mcking65 changed the title Coverage and Quality Report: dd forced-colors media query and currentcolor value to coverage report Coverage and Quality Report: Add reporting on use of forced-colors media query and currentcolor value Jun 18, 2024
@jongund
Copy link
Contributor Author

jongund commented Jun 18, 2024

@mcking65
I also removed the CSS :before, :after and content columns in the table.
Changed the name of the table to "SVG and High Contrast Techniques".
The use of CSS :before and :after are not part of the coding practices.

@mcking65 mcking65 requested a review from howard-e June 25, 2024 14:07
@mcking65
Copy link
Contributor

@howard-e
Why is Check examples/index.html / coverage failing?

I assume that is why the preview still shows the CSS columns Jon removed from the graphics techniques table; the generation script didn't run?

I requested re-review now that Jon has made requested updates.

@howard-e
Copy link
Contributor

howard-e commented Jun 25, 2024

@howard-e Why is Check examples/index.html / coverage failing?

This PR needs to manually re-run npm run coverage-report because the lint-staged step to automatically do it doesn't trigger when the generating script alone is updated in a PR. Instead, it was only being triggered when there are changes to content/patterns/**/examples/*.html.

But it is odd that the generating scripts weren’t being captured so I submitted #3048 to address it.

I assume that is why the preview still shows the CSS columns Jon removed from the graphics techniques table; the generation script didn't run?

Not exactly, this was because there was an invalid file read happening with the new experimental content support on the build repo because of an uncaught assumption that the newly created files in #2977 were already present when the build repo references w3c/aria-practices.

Already submitted w3c/wai-aria-practices#330 to fix and has been merged and and Jon’s PR preview has been regenerated

I requested re-review now that Jon has made requested updates.

Will check it!

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

@jongund thanks for addressing the feedback and the additional changes look good to me.

This still just needs a merge with main to include #3026, running npm run coverage-report and a commit and push to be ready.

mcking65 pushed a commit that referenced this pull request Jul 2, 2024
… script update (#3048)

Related to PR #3024

Without this change, using lint-staged, the npm run reference-tables and npm run coverage-report scripts are automatically ran when a git commit happens ONLY if files matching content/patterns/**/examples/*.html have been changed. The generating scripts have been overlooked in that automatic process.

This PR triggers the process when adds scripts/coverage-report.* and scripts/reference-tables.* are changed. This should reduce failures of the Check examples/index.html / coverage check if only those script files have been updated in a PR.
@mcking65 mcking65 merged commit cb74ba2 into main Jul 23, 2024
12 checks passed
@mcking65 mcking65 deleted the update/coverage branch July 23, 2024 06:23
@mcking65 mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy About Page Related to a page within the About section of the APG labels Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
About Page Related to a page within the About section of the APG 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.

4 participants