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

Improve the block mover buttons accessibility #557

Closed
afercia opened this issue Apr 28, 2017 · 15 comments
Closed

Improve the block mover buttons accessibility #557

afercia opened this issue Apr 28, 2017 · 15 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Status] In Progress Tracking issues with work in progress [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@afercia
Copy link
Contributor

afercia commented Apr 28, 2017

The block mover buttons are empty, there's no text or aria-label at all, thus there's nothing that can be announced by assistive technologies.
For screen readers, they could use hidden text or an aria-label. I'd recommend to test them also with speech recognition software, really not sure how they can be activated if they have no visible text at all.

Additionally, the color contrast is really low. As a reminder, the WCAG level AA WordPress aims for requires a color contrast ratio of at least 4.5:1.

screen shot 2017-04-28 at 17 38 20

About the SVG icons, see the specific issue #528

Of course, these buttons should be fully operable when using only a keyboard.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 28, 2017
@afercia
Copy link
Contributor Author

afercia commented Apr 28, 2017

Ideally, the button should use some text to say what it does but it should also somehow reference the block it relates to. I.e. "Move Up" and "Move Down" wouldn't help so much users if they don't know what they're going to move.

The component should grab some information about the currently edited block, I see the state has some references to the hoveredBlock and the selectedBlock, blocks have no labels or name though.

@aduth aduth added Design Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Task Issues or PRs that have been broken down into an individual action to take labels Apr 28, 2017
@njpanderson
Copy link
Contributor

Hi all - First time here so let me know if I'm doing anything wrong, but would like to attempt to take this issue on. Will be able to work on it from Sunday onwards.

Agreed that it's not exactly black and white as to how the block could be identified. Initial thoughts are to give the buttons labels which inherit from some portion of either the content or the type of content being moved.

Button contrast should be an easy enough thing to sort, assuming they're standard SVGs.

Let me know if there's anything I need to do to get started on this before Sunday but happy to take it on as a first task!

@aduth
Copy link
Member

aduth commented May 24, 2017

@njpanderson Would be great to have you take a look at this.

Since the BlockMover component is passed the uid of the block it will move, we could retrieve information about that block to help inform the user about what's to be moved. It's not clear to me exactly what information we'd want to convey: "Move Up: Text block" ? Relate it in some way to the element being moved?

We do have a label for a block available by its title property (example). The value returned by getBlock is only an instance of that block type (including its type, uid, and current attributes set). To get the title of a block, we can pass the block's type to getBlockSettings, which will return the registered block type information. (Aside: There's a related discussion at #842 about how we could clarify this distinction).

@aduth aduth added the [Status] In Progress Tracking issues with work in progress label May 24, 2017
@ellatrix
Copy link
Member

Since the BlockMover component is passed the uid of the block it will move, we could retrieve information about that block to help inform the user about what's to be moved. It's not clear to me exactly what information we'd want to convey: "Move Up: Text block" ? Relate it in some way to the element being moved?

Related: #552 and #873. I think this will be better when the buttons and toolbars are clearly linked to a given block. So there is the main tabbing flow of blocks, and you can list the relevant actions (movers, formatting, switcher) when pressing the shortcut. The user would know which block the button is for (the one they just pressed the shortcut for), and they press esc (or maybe action) to move the focus back. This flow is similar to the current core editor.

@ellatrix
Copy link
Member

Tbh, I think it's best to address this issue with the other block focus issues.

@njpanderson
Copy link
Contributor

Ok, so should I look into this discretely or would you guys rather roll this issue into a wider task? I am happy to take on something else if needed!

@aduth
Copy link
Member

aduth commented May 26, 2017

@njpanderson I'm not sure what specific tasks @iseulde had in mind, but I've made a pass through all of the issues to try to apply "Good First Task" to a number more, if you'd like to consider another option:

https://github.com/WordPress/gutenberg/issues?q=is%3Aopen+is%3Aissue+label%3A%22Good+First+Task%22+no%3Aassignee

@ellatrix
Copy link
Member

Sorry, I didn't mean to put you off. :) Rephrasing: I think it's best to address the question of what label we put on it with generally improving the flow for buttons that are attached to block. See #552.

@njpanderson
Copy link
Contributor

No problem, @iseulde - definitely not putting me off. Have been out of communications for the past two days whilst wandering around Arran. Will be likely having a look at the other issues @aduth posted tonight though, and see if there's something else a bit more standalone I can help with.

I've made a tiny change to the contrast of the buttons already and if you like I can PR that. Not sure it would be worth it though!

njpanderson added a commit to njpanderson/gutenberg that referenced this issue May 28, 2017
Have checked the new contrast meets WCAG AA using the following tool:

http://webaim.org/resources/contrastchecker/

If there’s a better checker you guys all use, do let me know in the comments!
@njpanderson
Copy link
Contributor

Ok, I've added something that leverages the label attribute of IconButton to show a (hopefully) helpful label which I've tested with Voiceover. It essentially says something like "Move [type] block from position [X] up to position [Y]".

Not sure how to let you all see this but it's currently sitting in a branch on my fork:
https://github.com/njpanderson/gutenberg/commits/update/block-mover-accessibility

Would like to add test case for that new generateOrderTitle function but not entirely sure what the process is there. It's also possible that the function would be more appropriately placed in a more generic location but let me know if that's the case.

njpanderson added a commit to njpanderson/gutenberg that referenced this issue May 29, 2017
Have checked the new contrast meets WCAG AA using the following tool:

http://webaim.org/resources/contrastchecker/

If there’s a better checker you guys all use, do let me know in the comments!
@njpanderson
Copy link
Contributor

That's test cases added for the function.

njpanderson added a commit to njpanderson/gutenberg that referenced this issue Jun 1, 2017
Have checked the new contrast meets WCAG AA using the following tool:

http://webaim.org/resources/contrastchecker/

If there’s a better checker you guys all use, do let me know in the comments!
njpanderson added a commit to njpanderson/gutenberg that referenced this issue Jun 1, 2017
After thinking about what would be best for the user in terms of accessibility and guidance, I’ve decided that trying to give them the “content” of a block wouldn’t very useful in a lot of contexts – especially non-text blocks.

Instead, I’ve opted for telling them where the block is, and where it’ll be moved to.

This is also the case for multiple selected blocks, where it will fall back to simpler language.
jasmussen added a commit that referenced this issue Jun 3, 2017
Increase colour contrast on block mover buttons (#557)
aduth added a commit that referenced this issue Jun 7, 2017
Add contextually aware title for block mover control (#557)
@nylen
Copy link
Member

nylen commented Jun 9, 2017

@afercia @njpanderson it looks like #981 and #984 are related. Is this issue now resolved, or is there still more to do here?

@afercia
Copy link
Contributor Author

afercia commented Jun 9, 2017

@nylen quickly tested with Safari+VoiceOver seems the positions are announced correctly and the color contrast is OK. Only doubt if the disabled up/down arrows when in first/last position should be really disabled but I'd lean towards keeping them as now so they're still focusable. In rare circumstances is acceptable to keep "disabled" controls focusable and seems to work better this way.
It would need to be tested a bit more with other screen readers and other assistive technologies but I think this issue can be closed! Can always open new ones for further refinements. Nice job! 🍷

@njpanderson
Copy link
Contributor

Thanks all. Daughter has been in hospital so it's been hard to keep up to date but great to see this issue now closed. On to the next thing!

@jasmussen
Copy link
Contributor

Oh my goodness! All the best thoughts to you and yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Status] In Progress Tracking issues with work in progress [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

7 participants