WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 4 months ago

Last modified 3 months ago

#47136 closed defect (bug) (fixed)

Image Editor: Reading and focus order do not match visual order

Reported by: anevins Owned by: audrasjb
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots wpcampus-report has-patch has-dev-note
Focuses: ui, accessibility Cc:

Description (last modified by afercia)

Moved from the WPCampus accessibility report issues on GitHub, see https://github.com/WordPress/gutenberg/issues/15315

  • Severity:
    • Medium
  • Affected Populations:
    • Blind
    • Low-Vision
    • Cognitively Impaired
  • Platform(s):
    • All / Universal
  • Components affected:
    • Edit Media

Issue description
On the Edit Media page, the keyboard focus order and the visual reading
order are presented in a zig-zag pattern.

Users who are not using browser zoom can reach an option to show more or
fewer columns:

1) If users select content to appear in a single column (which still
produces two columns visually), content at the top is first, then comes
content on the right (Scale and Crop sections), then content back to the
left (the actual image and editing buttons), and finally content
underneath both columns (metadata).

2) If users select content to appear in two columns (which produces
three columns visually), content at the top is first, then comes a
column in the middle ("Scale" and "Crop" sections), then the column
in the left (the actual image and editing buttons), and then the content
at the bottom (the default metadata). After some of the content at the
bottom, the "Save" section is on the far right, and then if the user
added more boxes, these are last and visually placed after the default
metadata content.

Keyboard users benefit from a logical focus order which is consistent
with the reading order. Users who have a cognitive disability and have
difficulty reading may also be confused by an unexpected or illogical
focus order.

Remediation Guidance
Either change the visible layout with CSS to make the visual order match
the content order, or re-order the content in the DOM to match the
visual order.

Relevant standards

Note: This issue may be a duplicate with other existing accessibility-related bugs in this project. This issue comes from the Gutenberg accessibility audit, performed by Tenon and funded by WP Campus. This issue is GUT-82 in Tenon's report

Attachments (9)

57182795-6f009f80-6e58-11e9-8165-1cdd43fd62df.png (176.1 KB) - added by anevins 18 months ago.
57182794-690abe80-6e58-11e9-8ea5-10883306a61f.png (274.9 KB) - added by anevins 18 months ago.
47136.diff (8.5 KB) - added by sabernhardt 13 months ago.
switches the order of two columns and sets both min-width and max-width for the first column
47136.1.diff (8.6 KB) - added by sabernhardt 4 months ago.
bugfix option: switch order of the two image editor columns, adjust spacing on mobile screens, change first focused element
image-edit-after-47136.1.diff-firefox-768x1024.png (1.2 MB) - added by sabernhardt 4 months ago.
after 47136.1.diff (Firefox 768x1024)
47136.2.diff (8.6 KB) - added by audrasjb 4 months ago.
New patch as the previous one doesn't apply anymore
560afbcad870c5b045a285e524561714.mp4 (2.3 MB) - added by audrasjb 4 months ago.
Tab order now matches the visual order
47136.3.diff (8.6 KB) - added by sabernhardt 4 months ago.
adding timeout to focus function
47136.4.diff (10.0 KB) - added by afercia 4 months ago.

Change History (64)

#1 @afercia
18 months ago

  • Description modified (diff)

#2 @afercia
18 months ago

  • Milestone changed from Awaiting Review to 5.3

#3 @afercia
17 months ago

Note: the Screen Options and Help buttons order was fixed in [45503] / #45094.

#4 @afercia
17 months ago

To clarify, the two screenshots from the WPCampus report attached above illustrate the two different layout options available in the "Screen Options" settings:

  • first screenshot: 1 column option (produces two columns visually)
  • second screenshot: 2 columns option (produces three columns visually)

Also, worth considering users can add additional meta boxes.

http://cldup.com/nPudKeOs4l.jpg

#5 @karmatosed
15 months ago

  • Keywords needs-design-feedback added

#6 @audrasjb
15 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


14 months ago

#8 @karmatosed
14 months ago

  • Keywords needs-design-feedback removed

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


14 months ago

#10 @afercia
14 months ago

  • Keywords needs-design added

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-media by afercia. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-media by afercia. View the logs.


13 months ago

This ticket was mentioned in Slack in #design by afercia. View the logs.


13 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


13 months ago

#16 @karmatosed
13 months ago

I chatted with @afercia about this today and for me, this indicates the need to consider a redesign from ground up of the media interface. I know many already feel this. If we can from the start consider a11y and the best possible interactions, then we aren't forcing something or hitting the limits of an existing design. A lot of friction in changes is happening as we are limited through the existing design confines.

I am aware, by saying this I am suggesting this happens not in 5.3. However, this ticket and others should be fuel to get a redesign sooner and at the heart of any consideration.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-media by afercia. View the logs.


13 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


13 months ago

#20 @afercia
13 months ago

  • Milestone changed from 5.3 to Future Release

Discussed during today's accessibility bug-scrub: giving WordPress 5.3 Beta 2 is very close, sadly agreed to punt this ticket to Future Release. This ticket needs a major redesign of the Image Editor an realistically we're out of time as in more than 4 months there was no progress.

#21 @sabernhardt
13 months ago

It's almost certainly too late to make a change like this for 5.3.0 and I (too) would like a better edit screen in 5.4 or later, but the following patch might be a good start to fixing the order in 5.3.1.

In image-edit.php, I simply moved the imgedit-panel-content container above the imgedit-settings <div> and then added wp-clearfix to the imgedit-settings <div> (even though I didn't notice a specific need for the class).

The stylesheet introduces a bottom padding on imgedit-panel-content for when it stacks and also sets both min-width and max-width when on larger screens. (400 pixels is currently the maximum width and height of any cropped image here.)

.wp_attachment_holder .imgedit-wrap .imgedit-panel-content {
	float: left;
	padding: 3px 16px 20px 0;
}

@media only screen and (min-width: 783px) {
	.wp_attachment_holder .imgedit-wrap .imgedit-panel-content {
		min-width: 400px;
		max-width: calc( 100% - 266px );
	}
}

Note about box-sizing:
I chose to subtract the right padding value from the max-width rather than setting the box-sizing property. If box-sizing: border-box is preferable, then the padding value needs to be added to the min-width instead.

@media only screen and (min-width: 783px) {
	.wp_attachment_holder .imgedit-wrap .imgedit-panel-content {
		box-sizing: border-box;
		min-width: 416px;
		max-width: calc( 100% - 250px );
	}
}

@sabernhardt
13 months ago

switches the order of two columns and sets both min-width and max-width for the first column

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


13 months ago

#23 @afercia
13 months ago

  • Milestone changed from Future Release to 5.4

#24 @sathyapulse
11 months ago

@sabernhardt Shall we include the CSS from 48780.3.patch in #48780 if it works fine? It covers more cases.

This ticket was mentioned in Slack in #core by sabernhardt. View the logs.


11 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


10 months ago

#27 @afercia
10 months ago

  • Summary changed from Reading and focus order do not match visual order to Image Editor: Reading and focus order do not match visual order

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


9 months ago

#29 @sabernhardt
9 months ago

Update: the patch added here is insufficient. Ideally we would redesign this screen to fix the order, as mentioned in comment:16.

Another possibility is that a fix on #48780 brings us close enough that we only need to switch the two columns' order to consider this bug fixed. If so, then we could open an enhancement ticket to focus on improving the overall design.

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


9 months ago

#31 @audrasjb
9 months ago

  • Milestone changed from 5.4 to 5.5

Moving to 5.5 milestone as beta 1 is tomorrow.

#32 @SergeyBiryukov
8 months ago

In 47418:

Media: Improve the appearance of image editor on small and medium screens.

This prevents the main area of Edit Media screen from being pushed down too far.

Props sabernhardt, afercia, fierevere, sathyapulse, mikeschroder, johnbillion.
Fixes #48780. See #47136.

#33 @SergeyBiryukov
8 months ago

In 47419:

Media: Improve the appearance of image editor on small and medium screens.

This prevents the main area of Edit Media screen from being pushed down too far.

Props sabernhardt, afercia, fierevere, sathyapulse, mikeschroder, johnbillion.
Merges [47418] to the 5.3 branch.
Fixes #48780. See #47136.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 months ago

#35 @afercia
6 months ago

Some initial redesign ideas were explored by @nrqsnchz see starting from https://core.trac.wordpress.org/ticket/47116#comment:11

Ideally, all the part related to that exploration should be moved to this ticket. See https://core.trac.wordpress.org/ticket/47116#comment:39

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 months ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 months ago

@sabernhardt
4 months ago

bugfix option: switch order of the two image editor columns, adjust spacing on mobile screens, change first focused element

#40 @sabernhardt
4 months ago

  • Keywords has-patch needs-testing added; needs-patch needs-design removed

47136.1.diff could fix the focus order within the current design.

  1. PHP: Move .imgedit-panel-content container before .imgedit-settings. The code for each section should remain exactly the same as it has been.
  2. CSS: Add 16 pixels of padding to the bottom of each container for screens up to 782 pixels wide. It's only necessary on .imgedit-panel-content, but I like it on both.
  3. JS: Adjust the focused element after the Edit Image button is activated/clicked. Focus moves to the first .imgedit-menu button (Crop) instead of to the first help button (Scale Image).

I like the idea of also dividing the settings groups into two columns when on narrower screens (from either 400 or 480 pixels up to 782), so the Image Crop box would be closer to the image to edit. If that might be desired, I could upload a patch to show that layout concept. However, I still would want this part committed before any additional adjustments.

@sabernhardt
4 months ago

after 47136.1.diff (Firefox 768x1024)

@audrasjb
4 months ago

New patch as the previous one doesn't apply anymore

@audrasjb
4 months ago

Tab order now matches the visual order

#41 @audrasjb
4 months ago

  • Keywords needs-testing removed

The previous doesn't apply anymore. 47136.2.diff refreshes @sabernhardt ’s proposal.

I tested the tab order with this patch and it looks good to me (see video screenshot above).

#42 follow-up: @sabernhardt
4 months ago

The patch's JavaScript edit was intended to move focus to the Crop button, but it seems the focus is going to something immediately before the Crop button. (This may be acceptable, though unexpected.)

I noticed this before testing 47136.2.diff and after creating my patch. It had worked before creating 47136.1.diff.

Last edited 4 months ago by sabernhardt (previous) (diff)

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 months ago

#44 in reply to: ↑ 42 @afercia
4 months ago

Replying to sabernhardt:

The patch's JavaScript edit was intended to move focus to the Crop button, but it seems the focus is going to something immediately before the Crop button. (This may be acceptable, though unexpected.)

I noticed this before testing 47136.2.diff and after creating my patch. It had worked before creating 47136.1.diff.

Testing 47136.2.diff focus does go to the Crop button for me. Looks correct. Does it still happen on your side?

#45 @sabernhardt
4 months ago

The JS focus shift might be less reliable with NVDA active and/or in Microsoft browsers. Or there's something wrong with my setup.

I reverted and updated my environment, then re-applied the patch. The focus moved to the Crop button again in Windows Chrome, Firefox, the new Edge and IE11.

After I reactivated NVDA:

  • IE consistently did not cooperate.
  • Edge had trouble with the focus a few times, yet not all.
  • Chrome did not focus on Crop once out of a few times (not the first time), but that was when I clicked the Edit Image button instead of using the Enter key or spacebar. (I might have clicked twice accidentally and removed the focus from the button.)
  • Firefox tests were successful. I'm sure I saw the issue in Firefox's Responsive Design Mode earlier today because I wondered if it was an issue with the mode or related to the smaller screen size. However, now Firefox is setting the focus consistently on Crop for me (whether regular or responsive mode, with or without the screen reader).

Deactivating NVDA again, Firefox and Chrome were both successful in further attempts. I still saw the focus was not on Crop once in Edge plus at least four times in IE, with some successful tries in each of those two browsers.

#46 @sabernhardt
4 months ago

I checked the focus with WordPress 5.4 and had similar results with the Scale Image help button there. So it's apparently nothing new and could be better as a separate ticket.

With the screen reader, I tested with random images and used either Enter or Space keys. After testing with NVDA, I was more methodical, activating the edit mode with each key for three separate images. (Spacebar was successful more often, though I'm not convinced the key mattered.)

IE + NVDA: 1/4
Edge + NVDA: 2/4
Firefox + NVDA: 3/5
Chrome + NVDA: 5/6

IE: Enter 1/3, Space 2/3
Edge: Enter 3/3, Space 3/3
Firefox: Enter 3/3, Space 3/3
Chrome: Enter 2/3, Space 3/3

#47 @sabernhardt
4 months ago

Well, the first search result on "jquery focus not working" helped. After setting a short timeout, the focus worked every time.

@sabernhardt
4 months ago

adding timeout to focus function

#48 @afercia
4 months ago

@sabernhardt thanks for your thorough testing. I tested it with Windows screen readers as well and you're right. Seems setting focus on the imgLoaded function wasn't a good idea, my fault.

That's not so reliable as it depends on browsers implementation, whether the image is cached or not, and also whether the browser's dev tools are open, which makes testing a bit misleading.

Regarding:

but it seems the focus is going to something immediately before the Crop button

Yep, I guess that's the effect of the so called "sequential focus navigation starting point". For Chrome, see https://bugs.chromium.org/p/chromium/issues/detail?id=454172. Firefox implemented it way before Chrome. Basically, when a portion of the DOM that contained focus is removed, modern browsers try to make an educated guess on the most reasonable place where to start the tab sequence from.

In our case, especially when screen readers are running, seems that the element to focus it not there yet, or at least the browser didn't update the accessibility tree yet. Thus, the "sequential focus navigation starting point" kicks in. Note that IE didn't ever implement this feature, which contributes to the different testing results.

Ideally, we should try to move focus on a more reliable callback or event. One that runs when the Image editor is fully rendered. Alternatively, I think your approach with setTimeout() is sae enough as the worst thing can happen is focus isn't moved (no potential JS errors).

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 months ago

#50 @afercia
4 months ago

  • Keywords needs-dev-note added

#51 @audrasjb
4 months ago

Let's add an information about that change in the "Miscellaneous changes" dev note

@afercia
4 months ago

#52 @afercia
4 months ago

  • Keywords commit added

I couldn't find a better way to determine when the image editor is "fully loaded" so the setTimeout() approach is good enough.

In 47136.4.diff:

  • moved the focus management out from imgLoaded() and into a new specific focusManager() function so that the functions responsibilities are clearer
  • abstracted the retrieval of the first element to move focus to by the means of jQuery UI :tabbable so that focus will always be placed on the first tabbable element within the image editor, whatever it is

#53 @afercia
4 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 48265:

Accessibility: Media: Fix the Image Editor mismatching keyboard focus order and visual reading order.

Swaps the DOM order of the two main columns within the admin Image Editor.

When the sequence in which content is presented affects its meaning and the navigation sequences affect meaning or operation, visual order and DOM order must match. See WCAG 2.1 Success Criterion 1.3.2 Meaningful Sequence and Success Criterion 2.4.3 Focus Order.

Props sabernhardt, anevins, audrasjb, afercia.
Fixes #47136.

#54 @sabernhardt
3 months ago

  • Keywords commit removed
Last edited 3 months ago by sabernhardt (previous) (diff)
Note: See TracTickets for help on using tickets.