Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#47131 closed defect (bug) (fixed)

Cursor suggests functionality even when functionality is not present

Reported by: anevins's profile anevins Owned by: antpb's profile antpb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots wpcampus-report has-patch
Focuses: ui, accessibility, javascript Cc:

Description

Users who mouse over the "Save" heading on the Edit Media page see a
"move" cursor, which suggests to users that the "Save" section can
be dragged and dropped to somewhere else. However, this is only the case
if additional boxes are shown, re-ordering is not possible if "Save"
is the only visible box (although the element can be dragged, there is
nowhere else to drop it).

The use of consistent and relevant cursors may be important for users
who have a cognitive disability, since cursors give a visual clue as to
an element's functionality. Using the move cursor for elements which
cannot be moved may be confusing or counter-intuitive for users.

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

Issue code

    .js .postbox .hndle, .js .widget .widget-top {


        cursor: move;


    }

Remediation Guidance
Only allow the move cursor to appear to users when re-ordering is
functionally possible.

Ideally, the drag and drop functionality should not be available at all
when only one box is present (i.e. it should not even be possible to
drag that one box), however this may be unrealistic to fix, and
therefore the solution of removing the cursor style will go a long way
towards matching user expectations.

Relevant standards
N/A

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-78 in Tenon's report

Attachments (8)

57181541-74c8a600-6e95-11e9-9129-95cc46fe0721.jpg (75.5 KB) - added by anevins 6 years ago.
panel cursor move.png (20.6 KB) - added by afercia 6 years ago.
47131.diff (8.8 KB) - added by adamsilverstein 6 years ago.
47131.2.diff (8.8 KB) - added by adamsilverstein 6 years ago.
47131.3.patch (2.5 KB) - added by antpb 5 years ago.
Uploading a refreshed patch without the package-lock.json changes. Still need to investigate the instances or sortable.
47131.4.patch (2.5 KB) - added by antpb 5 years ago.
changed the formatting of the doc block for the new function that checks if a meta box should be sortable
47131.3.diff (3.0 KB) - added by afercia 5 years ago.
47131.4.diff (3.0 KB) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (40)

#1 @anevins
6 years ago

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

Last edited 6 years ago by anevins (previous) (diff)

#2 @afercia
6 years ago

  • Component changed from General to Media

#3 @afercia
6 years ago

  • Milestone changed from Awaiting Review to 5.3

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


6 years ago

#5 @anevins
6 years ago

Suggestions from the Core Media meeting: https://wordpress.slack.com/archives/C02SX62S6/p1559224344044600

  • Worth investigating whether this is the case with other metaboxes and if so to apply a wider solution.
  • It has been suggested that this may be an issue in the classic editor and not just Media. The icon/dragging shouldn’t be enabled if there’s only one metabox present.
  • In common.js I see the jQuery-UI sortables are enabled for any $sortables = $('.meta-box-sortables'), It could be as simple as checking the length of the jQuery collection and return if 1
Last edited 6 years ago by anevins (previous) (diff)

#6 @afercia
6 years ago

Testing quickly the Edit Media page, it appears things are a bit more complicated.

This page, like other pages, can be set to a 1 or 2 columns layout in the Screen Options.

  • 2 columns: I can actually move the Save meta box to the bottom of the page (though I wonder how this can be useful)
  • 1 column: there's no place where to move the Save meta box, though it's still draggable

So it appears that a logic to enable / disable the jQuery UI sortables should take into account not only the amount of meta boxes in a page but also the layout option.

Last edited 6 years ago by afercia (previous) (diff)

#7 @afercia
6 years ago

Worth noting the same problem happens in Gutenberg with panels added by plugins. These panels reuse the markup from the legacy meta boxes. In the core files postbox.js and common.js there's some code that targets these panels and tries to attach behaviors for the jQuery UI sortable. Also, common.css sets the cursor style to cursor: move;.

I think there's a related issue on the Gutenberg GitHub repo. See attached screenshot.

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


6 years ago

#9 @adamsilverstein
6 years ago

This page, like other pages, can be set to a 1 or 3 columns layout in the Screen Options.
2 columns: I can actually move the Save meta box to the bottom of the page (though I wonder how this can be useful)
1 column: there's no place where to move the Save meta box, though it's still draggable

Based on this I am going to make meta boxes dragable if:

  • There is more than one meta box OR
  • The screen is in 2 column mode

#10 @adamsilverstein
6 years ago

  • Keywords has-patch added; needs-patch removed

in 47131.2.diff:

  • When the screen is in one column mode and there is only one meta box, disable sortables, otherwise enable
  • When the user toggles any screen option (columns, metas boxes) recheck and disable or enable sortables.
  • Set the cursor inline style to indicate draggability accordingly

Works well in my testing, I would appreciate some additional review.

#11 @adamsilverstein
6 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#12 @adamsilverstein
6 years ago

@afercia if you have a chance I would appreciate a review/test of 47131.2.diff

#13 @afercia
6 years ago

  • Keywords needs-refresh added

@adamsilverstein thanks for the patch and the reminder! :)

Note: the patch doesn't apply cleanly and also modifies the package-lock.json file, because of the https / http issue. I rebuilt it locally but not uploaded here as I think it's faster for you to regenerate it keeping the Git format (I use SVN).

Seems to me it's working fine. However, I think there are other places in the admin where the same fix should be used. For example: the edit post page, both in the Classic Editor and in Gutenberg.

Searching for occurrences of .sortable( I did find at least two cases worth checking, in:
src/js/_enqueues/admin/postbox.js
and
src/js/_enqueues/wp/dashboard.js

#14 @adamsilverstein
6 years ago

Thanks for the review @afercia - I'll take a look at those other spots as well.

#15 @adamsilverstein
6 years ago

  • Milestone changed from 5.3 to Future Release

Milestoning this as 'future release' until there is some more progress towards a complete solution.

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


6 years ago

#17 @antpb
6 years ago

Since all that is needed here is investigation into the other two instances of .sortable( I think it’s fair to include this in the 5.3 milestone. @adamsilverstein if you are okay with it, I can assign this to myself and make sure we get it across the line for 5.3.

#18 @adamsilverstein
6 years ago

  • Milestone changed from Future Release to 5.3
  • Owner changed from adamsilverstein to antpb
  • Status changed from assigned to reviewing

@antpb Absolutely - moving to 5.3 and assigning to you now. Feel free to ping me for testing or review. I mostly moved out of 5.3 because I wasn't sure I would find the time to work on the ticket.

Last edited 6 years ago by adamsilverstein (previous) (diff)

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


5 years ago

@antpb
5 years ago

Uploading a refreshed patch without the package-lock.json changes. Still need to investigate the instances or sortable.

#20 @antpb
5 years ago

  • Keywords needs-refresh removed

Just reporting that my testing confirms that the patch still fixes the issue and that also the dashboard.js instances of sortable are respecting the width condition of the newly introduced maybeDisableSortables function and do not allow movement when in the one column width less than 782.

For visibility, here is the logic that controls if sortable is applied based on width and column prefs && number of boxes:

			if (
				( width <= 782 ) ||
				( 1 >= $sortables.find( 'h2:visible' ).length && jQuery( '.columns-prefs-1 input' ).prop( 'checked' ) )
			) {
				this.disableSortables();
			} else {
				this.enableSortables();
			}

I have one more instance to verify in postbox.js Will hopefully have some good news soon, but this is looking super close to being ready for a final review.

#21 @antpb
5 years ago

  • Keywords needs-docs added

I think this will need docs on maybeDisableSortables() Working on that now. new patch to come soon!

@antpb
5 years ago

changed the formatting of the doc block for the new function that checks if a meta box should be sortable

#22 @antpb
5 years ago

  • Keywords commit added; needs-docs removed

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


5 years ago

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


5 years ago

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


5 years ago

#26 @afercia
5 years ago

Thanks all! Reviewing a bit the patch, a couple considerations:

  • Targeting the h2 element with find( 'h2' ) could expose to failure with potential, future, changes in the markup. Wouldn't be safer to target the .ui-sortable-handle class?
  • I'd consider to add/remove a CSS class instead of using jQuery .css()

More importantly, the script doesn't update dynamically "draggability" and cursor style when changing meta boxes visibility in the Screen Options. To reproduce, for example:

  • edit an attachment in the Edit Media page
  • set 1 column in the Screen Options
  • unset visibility for all the meta boxes
  • the only visible meta box is Save with cursor: auto, which is correct
  • in the Screen Options make one more meta box visible
  • at this point there are two visible meta boxes and they should be sortable
  • instead, they can't be dragged and the cursor style is still auto
  • refresh the page
  • see that now the two meta boxes are sortable

I see there's a custom event $document.trigger( 'postbox-toggled', $postbox ); that could be used. Will try to update the patch.

#27 @afercia
5 years ago

  • Keywords commit removed

@afercia
5 years ago

#28 @afercia
5 years ago

  • Keywords commit added

47131.3.diff addresses the points above.
@antpb @adamsilverstein I'd appreciate your quick review :) Changes are pretty straightforward.

#29 @adamsilverstein
5 years ago

Looks good, nice improvements @afercia!

@afercia
5 years ago

#30 @afercia
5 years ago

@adamsilverstein thanks for the review!

47131.4.diff refreshes the patch after recent line numbers changes.

#31 @afercia
5 years ago

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

In 46250:

Accessibility: Make sortable meta boxes non sortable when there are no locations they can be dragged to.

Depending on the amount of meta boxes and the layout settings under Screen Options, sortable meta boxes may not be actually sortable.
In these cases, jQuery UI sortable needs to be disabled and the user interface shouldn't use a CSS cursor: move.

The use of consistent and relevant cursors may be important for users who have a cognitive disability, since cursors give a visual clue as to an element's functionality. Using the move cursor for elements which cannot be moved may be confusing or counter-intuitive for users.

Props adamsilverstein, antpb, anevins.
Fixes #47131.

#32 @afercia
5 years ago

  • Keywords commit removed
Note: See TracTickets for help on using tickets.