Remove role group from blocks to see if I can fix JAWS. #32799
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alexstine! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
github-actions
bot
added
the
First-time Contributor
alexstine
added
[Type] Bug
|
Right now I am waiting on PR testing. I believe this is in a good spot for code review. I still have some broken e2e tests, but I will fix them tomorrow when I am more awake. Want to know if anyone foresees any issues with this change. Thanks. |
|
Do we know when this role was added and why? |
|
I think it was introduced via #19213. |
|
It would be easier if there was a way to only target JAWS for example, but we're not there yet. Compatibility between screen readers is not an easy thing to achieve. When I removed the role, I couldn't notice NVDA announcing anything less or more, I wonder if it is required now. The down side is I am running the latest version so it is possible it could break in older versions of NVDA for example. I still lack a bit of background as to why this was introduced in the first place. |
|
Maybe @MarcoZehe could shed some light on this if time allows. |
| id: `block-${ clientId }${ htmlSuffix }`, | ||
| tabIndex: 0, | ||
| role: 'group', | ||
| role: undefined, |
There was a problem hiding this comment.
Why not just remove the property entirely (I mean dropping this line), I guess same thing for the tests.
There was a problem hiding this comment.
@youknowriad I didn't remove it because rich-text defines a default role of textbox. Specifying undefined here overrides the role in passed props.
Either way, the tests would have needed changing. Seems this effects quite a few of them.
There was a problem hiding this comment.
so we don't want to retain the textbox role for RichTexts? Why some RichText should have it and others not?
There was a problem hiding this comment.
I don't really see the need for it. contentEditable seems to work fine unless others see a specific reason we should retain this role. It will need a fair amount of testing to be sure.
There was a problem hiding this comment.
Fair enough, I think this was originally proposed by @afercia. We'd have to dig up the history to check the reasoning there.
|
@ellatrix This PR might be of interest to you as well. |
|
Sorry, I am currently not available for any accessibility work due to being ill. |
|
@MarcoZehe sorry to here that, Get well soon ❤️ |
…ix/jaws-editing-blocks
|
@youknowriad Fixing the e2e tests would be much faster if it didn't error out on my computer. https://wordpress.slack.com/archives/C02QB2JS7/p1623986632256700 Any suggestions on fixing that? Get well @MarcoZehe |
|
@youknowriad I fixed a lot of the e2e tests but can't figure out these remaining ones. Can you provide any assistance? Thanks. |
|
I think the two remaining ones are independent from this PR and have been inconsistently failing in trunk as well. We're working on them separately, you can try rebasing the PR and/or restarting the jobs to see if they fail consistently here. |
…ix/jaws-editing-blocks
|
Just merged upstream trunk in to this PR. Hopefully it will fix it. Good to know those checks were not associated with this PR. I thought I had fixed them all from local tests. |
|
This needs more work. I am moving my work to another branch and will open a fresh PR when ready. |
4 participants
Description
Resolves #29526
Remove
role="group"from block props to hopefully fix #29526.How has this been tested?
I local tested this with NVDA screen reader and all seems to be working fine.
Screenshots
Types of changes
Bug Fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).