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

Check possible_heading rule with computedStyles #622

Merged
merged 17 commits into from
May 24, 2024

Conversation

pattonwebz
Copy link
Collaborator

@pattonwebz pattonwebz commented May 14, 2024

This PR moves the possible_heading rule to a JS rule that can check element styling with computedStyles. That prevents any need to parse CSS vars, functions or pseudo-classes since it gets the actual styles the element has after all the cascade is applied.

@amberhinds I will need some help setting the tags that the rule should have.

Sidenote: I like to see PRs that make things more powerful yet are red than green (or orange and blue in my case haha) :)
Screenshot from 2024-05-14 22-18-28

Fixes: #612

@amberhinds
Copy link
Member

@pattonwebz what do you mean by tags?

@pattonwebz
Copy link
Collaborator Author

@amberhinds sorry I should have linked to the list. I need to know the correct tags and a category from the list here: https://www.deque.com/axe/core-documentation/api-documentation/#axecore-tags

I presume this is the list but let me know if it misses anything or something shuoldn't be in the list:

wcag2a
wcag2aa
wcag21a
wcag21aa
wcag22aa
wcag131
wcag241
wcag246

@amberhinds
Copy link
Member

amberhinds commented May 15, 2024

Got it. Here are the tags:

  • wcag2a
  • wcag131
  • wcag241
  • cat.semantics

My understanding of "Each rule has one tag that indicates which WCAG version / level it belongs to" is that we don't need wcag2aa, wcag2aaa, etc. because if you were to query for AAA you would just query all three (wcag2a, wcag2aa, and wcag2aaa). Likewise, 2.2 includes 2.1 and 2.0; we want the tag to more accurately reflect what version and level the specific rule is.

@amberhinds
Copy link
Member

@pattonwebz
Copy link
Collaborator Author

@pattonwebz did you use this? dequeuniversity.com/rules/axe/4.9/p-as-heading?application=RuleDescription

I didn't use that rule because it doesn't work as well as what we did in PHP. I replicated our own rule instead of using this.

The rule from axe-core doesn't check as many things as we do (it doesn't care about font size), and in my testing, the things it found were marked as incomplete, which means it needs additional review to determine a pass or fail. It can't tell anything with certainty from the data I have run it against — probably why it's still flagged as an experimental rule.

Screenshot from 2024-05-15 14-56-09

Our rule catches 8 items, and the axe-core rule catches 0 and returns a 'can't tell' result with only one item from my test data.

@SteveJonesDev SteveJonesDev added this to the v1.13.0 milestone May 15, 2024
@amberhinds
Copy link
Member

Thanks for that info! Sounds like our rule is better. I wonder if we need to document anywhere when our rule is better than an axe rule so we remember that for the future.

@pattonwebz
Copy link
Collaborator Author

I can add some comments in the code for this rule so that we can remember the next time we touch the code. I'll see if I can find a place to document it outside of the code for ourselves to and form the basis of some copy for the plugin docs that explains why our check does certain things that axe-core misses.

@amberhinds
Copy link
Member

Code comments sound good. Outside the code tracking here makes sense: https://docs.google.com/spreadsheets/d/10vbbz4Y4uit76z73U8ffa9h6NN_ha5TC6Vfkt2-4IxI/edit?usp=sharing

src/pageScanner/helpers/helpers.js Outdated Show resolved Hide resolved
src/pageScanner/helpers/helpers.js Outdated Show resolved Hide resolved
export default {
id: 'paragraph_styled_as_header',
evaluate: ( node ) => {
const pixelSize = fontSizeInPx( node );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what happens before this but is there any value in returning early here if node is empty?

if ( ! node ) {
     return false;
}

src/pageScanner/checks/paragraph-styled-as-header.js Outdated Show resolved Hide resolved
src/pageScanner/checks/paragraph-styled-as-header.js Outdated Show resolved Hide resolved
src/pageScanner/checks/paragraph-styled-as-header.js Outdated Show resolved Hide resolved
src/pageScanner/rules/possible-heading.js Outdated Show resolved Hide resolved
@pattonwebz pattonwebz merged commit c7b2130 into develop May 24, 2024
16 checks passed
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.

Possible heading needs to support CSS variables
3 participants