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

Fix/issue 12501 menu item aria label #12955

Merged
merged 1 commit into from Feb 1, 2019

Conversation

Projects
None yet
4 participants
@timwright12
Copy link
Contributor

timwright12 commented Dec 17, 2018

Description

Fixes #12501

How has this been tested?

  • Local unit tests
  • Local e2e tests
  • Browser testing

Screenshots

screen shot 2018-12-17 at 1 34 59 pm

Types of changes

Removed aria-label on the menuItem component because it was redundant with the button text itself. Also update the tests and snapshots to match the new output.

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.
@timwright12

This comment has been minimized.

Copy link
Contributor Author

timwright12 commented Dec 17, 2018

Can someone confirm if the test fails are legitimate? They seem like unrelated items, but if they aren't I can get back in there

cc @tofumatt

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

Hi @timwright12 thank you for your contribution!

I tested these changes on Mac with voice over. I verified that for menu items with keyboard shortcuts like "Code Editor" and "Visual Editor" in the ellipsis menu, previously voice over just announced the text, now voice-over announces the keyboard shortcuts too.
I guess keyboard shortcuts may also be useful for voice over users so this may even be an improvement, but I'm not sure if this is the right way to announce the shortcuts or if it is expected. cc: @afercia

Regarding the end 2 end tests unfortunately previously we had problems that block end 2 end execution, would it be possible to rebase this branch so the end 2 end tests cannot be executed?

@timwright12

This comment has been minimized.

Copy link
Contributor Author

timwright12 commented Dec 20, 2018

@jorgefilipecosta can you have another look at the tests, I just pulled in the latest master. I updated the snapshots as well.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

Hi @timwright12 I investigate the tests problem and I think the root cause of the issue is that we have a test util that relies on this aria labels being present.
I think during the snapshot update as the util was not updated the snapshots got broken.
I created a PR that fixes the test util to not rely on this aria labels #13166. As soon as this PR is merged I think we should be able to merge this PR without any changes to the end 2 end tests snapshots.

`;

exports[`List can be created by typing an asterisk in front of text of a paragraph block 2`] = `
"<!-- wp:paragraph -->

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 2, 2019

Member

This test ensures that a list block should be created, now we are updating the snapshot to specify that a paragraph can be created. I think something went wrong during the snapshot creation.

@gziolo
Copy link
Member

gziolo left a comment

This should be unblocked as soon as @jorgefilipecosta merges #13166. This PR will need to be updated with the latest changes from master when that happens and all changes from test files should be removed.

Otherwise, the change looks good 👍

jorgefilipecosta added a commit that referenced this pull request Jan 31, 2019

Make clickOnMoreMenuItem not dependent on aria labels (#13166)
The clickOnMoreMenuItem end 2 end test util was dependent on aria labels being present on each menu item.
This aria labels are redundant and just duplicate the inner text menu item, they will be removed as soon as #12955 is merged.
This commit updates an end 2 end test util that relied on this aria labels being present.
This way we will make sure tests pass on #12955 as soon as this PR is merged.

## How has this been tested?
I verified the end 2 end tests pass.
I reverted all snapshot updates in #12955 and merge that PR with this and I checked the end 2 end tests continue to pass.
@timwright12

This comment has been minimized.

Copy link
Contributor Author

timwright12 commented Feb 1, 2019

@gziolo merged in latest master and refreshed the snapshots! Looks like there are some failure, but I can't tell if they're related or not. My e2es aren't running locally, just timing out.

pulled back package lock changes (+6 squashed commits)
Squashed commits:
[45f9e1d18] regen e2e tests
[0c693404d] reverted extra ticket content
[9a274107c] remove comments
[a238724c7] removed aria label from menuItem component
[a72a05183] darked focus state outline for block lists
[a8a125dfb] local save

@jorgefilipecosta jorgefilipecosta force-pushed the timwright12:fix/issue-12501-MenuItem-aria-label branch from ef69294 to 0bccf61 Feb 1, 2019

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

I updated the PR rebased it against master.
The test failures were being caused by unnecessary snapshot changes that I reverted. I think this is ready to be merged, thank you @timwright12 for the PR and @gziolo for the review.

comment was addressed

@jorgefilipecosta jorgefilipecosta merged commit 2b7085a into WordPress:master Feb 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment