-
Notifications
You must be signed in to change notification settings - Fork 86
Add tabindex to all grid row elements(with/without tabs) which has only plain text to make it tabable. #997
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: openj9
Are you sure you want to change the base?
Conversation
f4fbf77 to
782fb6a
Compare
|
@pshipton As per your suggestion here ibmruntimes/openj9-openjdk-jdk17#472, raising this PR. Thank you. |
|
Once the PR is merged, will backport the changes to 24/21/17 repos. Thank you. |
|
Personal build is successful with the changes and violation(The scrollable element with non-interactive content is not tabbable) is not seen with the changes in accessibility testing.
@smoinud1 FYI |
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Table.java
Outdated
Show resolved
Hide resolved
keithc-ca
left a comment
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.
Please avoid "tababble" - if it's a word, it would end with "able" (just one "b", not two).
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Table.java
Outdated
Show resolved
Hide resolved
…able Signed-off-by: Pasam Soujanya <psoujany@in.ibm.com>
9626cb3 to
9044930
Compare
| if (!matchFound) { | ||
| cell.put(HtmlAttr.ROLE, "tablist") | ||
| .put(HtmlAttr.TABINDEX, "0"); | ||
| } |
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.
These lines are redundant; see lines 338-341.
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.
Yes Keith, here we have 2 styles.
- rowStyle without tabs : cell.addStyle(rowStyle); creates div as
<div class="col-first even-row-color"> - rowStyle with tabs : cell.addStyle(tabClass); creates div as
<div class="col-last odd-row-color all-classes-table all-classes-table-tab2">
For both the styles tab should get focus, to do that we need to add role=tablist tabindex=0.
| if (!matchFound) { | ||
| cell.put(HtmlAttr.ROLE, "tablist") | ||
| .put(HtmlAttr.TABINDEX, "0"); | ||
| } |
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.
This seems to apply to many more places than the title suggests - I don't think this is what we want.
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.
These values should be applied to all grid rows with/without tab elements which has only plain text.
This PR fixes the accessibility violations seen with achecker tool.