-
Notifications
You must be signed in to change notification settings - Fork 138
[html-aam] Set heading level based on HTML computed heading level #2598
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for wai-aria ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Should the WebKit bug link to: https://bugs.webkit.org/show_bug.cgi?id=295092? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't make the meeting today, so wanted to get in a quick review. thank you again for working on this.
<td> | ||
<a class="core-mapping" href="#role-map-heading">`heading`</a> role, with the <a class="core-mapping" href="#ariaLevel">`aria-level`</a> property set to the number in the element's tag | ||
name. | ||
<a class="core-mapping" href="#role-map-heading">`heading`</a> role, with the <a class="core-mapping" href="#ariaLevel">`aria-level`</a> property set to the <a data-cite="html/sections.html#get-an-element's-computed-heading-offset">the computed heading level</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial reaction to this is "awesome! this is moving forward!"
But, in regards to how these new features should be specified in this spec, it might be best to combine the proposed updated text with the original text. e.g., indicate that the default level of a heading element would correlate to the number used in the tag name. But, the computed heading level of the element can be adjusted if also used with the new headingoffset and headingreset attributes.
Then those attributes would need to be added to the attributes table (so then their mention in the heading element mapping table can then in-doc link to those attributes) indicating how they can adjust the heading level (aria-level) mapping for in-scope heading elements.
Probably also worth adding a note stating that specifying an aria-level
attribute will take priority over whatever the implicit aria-level would be exposed as.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the HTML-AAM specification to use the HTML computed heading level algorithm instead of simply using the number in the heading element's tag name for setting the aria-level
property. This change aligns with recent HTML specification updates that introduce a headingoffset
feature for computing heading levels.
Key changes:
- Updates the mapping for h1-h6 elements to reference the HTML spec's computed heading level algorithm
- Replaces direct tag name number usage with a more sophisticated heading level calculation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
<td> | ||
<a class="core-mapping" href="#role-map-heading">`heading`</a> role, with the <a class="core-mapping" href="#ariaLevel">`aria-level`</a> property set to the number in the element's tag | ||
name. | ||
<a class="core-mapping" href="#role-map-heading">`heading`</a> role, with the <a class="core-mapping" href="#ariaLevel">`aria-level`</a> property set to the <a data-cite="html/sections.html#get-an-element's-computed-heading-offset">the computed heading level</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an extra "the" in the phrase "set to the the computed heading level". It should be "set to the computed heading level".
<a class="core-mapping" href="#role-map-heading">`heading`</a> role, with the <a class="core-mapping" href="#ariaLevel">`aria-level`</a> property set to the <a data-cite="html/sections.html#get-an-element's-computed-heading-offset">the computed heading level</a>. | |
<a class="core-mapping" href="#role-map-heading">`heading`</a> role, with the <a class="core-mapping" href="#ariaLevel">`aria-level`</a> property set to <a data-cite="html/sections.html#get-an-element's-computed-heading-offset">the computed heading level</a>. |
Copilot uses AI. Check for mistakes.
d90922c
to
9ac86a9
Compare
Refs whatwg/html#11086.
This wires up HTML-AAM to the
headingoffset
feature of the html spec which defines an algorithm for computing a heading level.Test, Documentation and Implementation tracking
Once this PR has been reviewed and has consensus from the working group, tests should be written and issues should be opened on browsers. Add N/A and check when not applicable.