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

Remove aria-label from block inserter list item #15382

Merged

Conversation

@brentswisher
Copy link
Contributor

commented May 1, 2019

Description

Removed an aria-label that was flagged as unnecessary in item WUT-18 of the WPCampus accessibility audit. Fixes #15337.

Note:
There is a code comment from @ellatrix that the aria-label was added to handle IE11 and JAWS2018. I do not have access to that configuration, so it should be tested by someone who does before this is merged to prevent a possible recursion.

In my opinion, if the aria-label makes it work in IE11/Jaws that should override the potential issue brought up in the audit and it should stay.

How has this been tested?

Tested with Apple VoiceOver on a Mac and it still reads out the button text correctly when navigated to with the keyboard.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@gziolo
Copy link
Member

left a comment

It looks like a big number of e2e tests depend on the aria-label attribute removed in this PR. You can run npm run test-e2e to see more details. I think there are a few places where it would have to be updated to make it pass. This label is also present in the span so it should be a straightforward change.

@ellatrix

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@brentswisher I didn't add this code. I just moved it. After tracking it down, it turns out it was added in #5828.

@gziolo gziolo requested a review from afercia May 6, 2019

@afercia

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

That comment was added by me 🙂 See #15337 (comment)

The aria-label and the visible text use the same variable. Originally introduced in #5828 to fix an issue specific to Internet Explorer 11 and JAWS, see #5827. Should be tested again to make sure it's still relevant.

@afercia
Copy link
Contributor

left a comment

Tested again with Internet Explorer 11 and JAWS 2018. Looks like the aria-label fix is no longer necessary. From an accessibility perspective can be removed safely.
Pending fixes for the tests :)

@talldan

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Hey @brentswisher, it looks like you might have pushed a branch masterer related to this PR:
https://github.com/WordPress/gutenberg/tree/masterer

Not sure how it happened 😄. Is it something that can be deleted?

@brentswisher

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

That's weird, I have no idea where that came from or how I managed to get it in the main Gutenberg repo! It's obviously a typo of some kind on my part, but I don't seem to have permission to delete it. If you could remove it and hide my shame from the world @talldan it would be much appreciated!

@brentswisher brentswisher force-pushed the brentswisher:update/remove-unnecessary-aria-label branch from cf7fd3b to bf06533 May 9, 2019

@brentswisher brentswisher requested review from aduth, ajitbohra, nerrad and ntwb as code owners May 9, 2019

@talldan

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I deleted it. No-one will ever know 😄

brentswisher added some commits May 1, 2019

Remove aria-label from block inserter list item
Because the text in the button is always visible, there
is no need ot add the aria-label. Having duplicated aria tags
increases the chance of a conflict with the visible text from
future changes. Brought up in item GUT-18 of the wpcampus
accessibliity audit.
Add waitForSelector to ensure the block navigator is open before atte…
…mpting to space into the first paragraph

@brentswisher brentswisher force-pushed the brentswisher:update/remove-unnecessary-aria-label branch from bf06533 to 5fafed9 May 10, 2019

@brentswisher

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@gziolo I've added fixes for the end-to-end tests. All the tests pass now, but I haven't worked with the e2e test suite much before, so a review would be very appreciated to see if there is a better way to do this :)

I contemplated refactoring into a 'clickBlockInserterButton' utility, but most tests use insertBlock anyways so it seemed a little overkill.

The third commit fixes an issue in block-hierarchy-navigation.test.js, but I am not sure if that is related to my changes or not. I was experiencing it intermittently locally on my master branch as well, but it consistently happened in my new branch.

I watched the test run in non-headless mode to see what was happening and it...passed. I watched again without slowmo enabled and it appeared the space was getting clicked before the block navigation opened. I wasn't sure if I should open another issue or commit it here, but hopefully that gives some background at least.

@@ -96,6 +96,7 @@ describe( 'Navigating the block hierarchy', () => {

// Return to first block.
await openBlockNavigator();
await page.waitForSelector( '.editor-block-navigation__container' );

This comment has been minimized.

Copy link
@gziolo

gziolo May 13, 2019

Member

I think I have PR where I included the same logic inside openBlockNavigator, I can refactor once I work on merging my own PR :)

@gziolo

gziolo approved these changes May 13, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@brentswisher, excellent work refactoring all those existing e2e tests to ensure they still pass with this aria-label attribute removed 👍

@gziolo gziolo merged commit c7a8d04 into WordPress:master May 13, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.