Skip to content

Make inline image toolbar focussable by keyboard#14001

Closed
ellatrix wants to merge 2 commits intotrunkfrom
try/inline-image-a11y
Closed

Make inline image toolbar focussable by keyboard#14001
ellatrix wants to merge 2 commits intotrunkfrom
try/inline-image-a11y

Conversation

@ellatrix
Copy link
Copy Markdown
Member

@ellatrix ellatrix commented Feb 21, 2019 •
edited
Loading

Description

Attempts to fix #10595. I added a keyboard shortcut to focus the contextual toolbar, currently only for inline images, but ideally, it should work the same for any contextual toolbar such as the one for links and ones added by plugins e.g. color. Also tells the user when on image gets selected and deselected, with the shortcut to focus the contextual toolbar.

Not expecting to be perfect from the start. Once it looks good I'll add an e2e test for it to avoid regressions.

N.B. There are boundaries around the image, which is a bug that will get fixed by #13948.

How has this been tested?

In a rich text field, add an inline image. Select it. You should get a spoken message that the image has been selected and which shortcut to use to access the contextual toolbar. Try it. The popover content wrapper should receive focus. You can now tab to the form elements. Edit the width and apply. Selection should go back to the rich text field. Deselect the image and there should be a spoken message again.

Screenshots

Types of changes

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.

@ellatrix ellatrix added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Feb 21, 2019
@ellatrix ellatrix requested a review from afercia February 21, 2019 12:05
@gziolo gziolo added this to the 5.2 (Gutenberg) milestone Feb 21, 2019
@gziolo gziolo added the [Package] Format library /packages/format-library label Feb 21, 2019
afercia
afercia requested changes Feb 21, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for working on this. After some testing I'm not sure I'm fully convinced the interaction is ideal though. Especially selection and initial focus. Will comment on the related issue for general discussion.

In the meantime, I've noticed a few issues:

1
When the inline image is in a new line (Shift+Enter) and then moving the caret across the image with the arrow keys, the block crashes (at least in Chrome): Cannot read property top of undefined

screenshot 2019-02-21 at 15 12 36

2
pressing Escape should close the inline toolbar and move focus back to the previous insertion point/selected image

Testing with Safari and VoiceOver:

3
the image is announced as selected even when it's not "visually" selected and the caret is still before the image. I guess this is because of the TinyMCE boundaries?

screenshot 2019-02-21 at 15 08 10

4
Can't reproduce consistently but trying to select the image and change its size multiple times can produce a duplicated image:

screenshot 2019-02-21 at 15 09 35

@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Feb 21, 2019

3
the image is announced as selected even when it's not "visually" selected and the caret is still before the image. I guess this is because of the TinyMCE boundaries?

4
Can't reproduce consistently but trying to select the image and change its size multiple times can produce a duplicated image:

These issues will be fixed by #33401.

2
pressing Escape should close the inline toolbar and move focus back to the previous insertion point/selected image

Agreed. It should select the image at least.

@ellatrix ellatrix mentioned this pull request Feb 21, 2019
Merged
5 tasks
This was referenced Feb 21, 2019
Closed
Closed
@afercia
Copy link
Copy Markdown
Contributor

afercia commented Feb 21, 2019

Agreed. It should select the image at least.

See also #14026

@ellatrix ellatrix removed this from the 5.2 (Gutenberg) milestone Mar 4, 2019
@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Apr 24, 2019
@guarani
Copy link
Copy Markdown
Contributor

guarani commented Oct 27, 2020

Looks like this needs to be rebased with master and conflicts resolved to be testable on mobile since this PR pre-dates the monorepo changes.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Nov 28, 2020
Base automatically changed from master to trunk March 1, 2021 15:42
@ellatrix
Copy link
Copy Markdown
Member Author

ellatrix commented Mar 9, 2021

Needs to be rethought and refreshed, so closing.

@ellatrix ellatrix closed this Mar 9, 2021
@ellatrix ellatrix deleted the try/inline-image-a11y branch March 9, 2021 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afercia afercia afercia requested changes

@aduth aduth Awaiting requested review from aduth

@gziolo gziolo Awaiting requested review from gziolo

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@youknowriad youknowriad Awaiting requested review from youknowriad

@guarani guarani Awaiting requested review from guarani

Assignees

No one assigned

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Format library /packages/format-library [Status] In Progress Tracking issues with work in progress [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inline images can't be resized using only the keyboard

4 participants

@ellatrix @afercia @guarani @gziolo