Make WordPress Core

Opened 5 months ago

Last modified 5 weeks ago

#60548 accepted defect (bug)

Image editor: improve the browsePopup function

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.7 Priority: normal
Severity: normal Version: 6.3
Component: Media Keywords: has-screenshots has-patch
Focuses: accessibility, javascript Cc:

Description

While auditing all the remaining jQuery deprecations still to address in core, I noticed a couple things that offer room for improvements in the browsePopup function of the core image editor.

  1. The browsePopup function uses the window.event property, which is deprecated and should not be used. This is the global event. Instead, the event should be passed as a function parameter. Aside: instead of using inline events, it could have been better to consider a more modern approach.
  2. When using the keyboard to navigate the items in the 'Image rotation' dropdown menu, the page scrolls. See attached animated GIF. There's some code in place to prevent page scrolling you may need to reduce your viewport height.

See [55919] / #50523

Attachments (2)

scroll.gif (1.2 MB) - added by afercia 5 months ago.
scroll-fix.gif (404.3 KB) - added by deepakvijayan 5 months ago.
after applying the fixes

Download all attachments as: .zip

Change History (15)

@afercia
5 months ago

#1 @afercia
5 months ago

Some more details:

  • The event used here, here, and here is the gloabl window.event, which is deprecated.
  • The preventDefault() used here doesn't actually prevent the page scrolling default action because the event in use is keyup which fires when a key is released. At that point the page scroll already occurred when pressing the key.
  • As a minor improvement, I'd suggest to rename the $target variable as that's not a jQuery object wrapping a DOM object. It is either false or the value returned by jQuery get() which is the DOM object. Traditionally, the core JavaScript prefixes variable names with $ as a convention for jQuery objects while this is a 'native' DOM object.
Last edited 5 months ago by afercia (previous) (diff)

#2 @deepakvijayan
5 months ago

Adjusting the event handling from onkeyup to onkeydown and passing the event object (event) as a parameter to the browsePopup function should help resolve the issues right?

Using onkeydown would indeed prevent the page from scrolling down as well.

For example inside image-edit.php if we change the following

<button type="button" class="imgedit-rleft button" onkeyup="imageEdit.browsePopup(this)" onclick="imageEdit.rotate( 90, <?php echo "$post_id, '$nonce'"; ?>, this)" onblur="imageEdit.monitorPopup()"><?php esc_html_e( 'Rotate 90&deg; left' ); ?></button>

to

<button type="button" class="imgedit-rleft button" onkeydown="imageEdit.browsePopup(event, this)" onclick="imageEdit.rotate( 90, <?php echo "$post_id, '$nonce'"; ?>, this)" onblur="imageEdit.monitorPopup()"><?php esc_html_e( 'Rotate 90&deg; left' ); ?></button>

similarly for all the buttons should help.

Also browsePopup could receive the event attribute like below

browsePopup: function (event, el) {
...

Also, we won't have to worry about depending on the global window.event.

I did test this out on my local environment and seems to be working fine.

@deepakvijayan
5 months ago

after applying the fixes

#3 @afercia
5 months ago

Adjusting the event handling from onkeyup to onkeydown and passing the event object (event) as a parameter to the browsePopup function should help resolve the issues right?

yes, either that or we would need to add a keydown event for the only purpose to prevent scrolling. It depends whether keeping the interaction with the menu on keyup is desired or not. I'd defer to @joedolson for a decision about the preferred interaction.

#4 @joedolson
5 months ago

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

Re: "Aside: instead of using inline events, it could have been better to consider a more modern approach. "

I didn't see any reason to have everything else in inline events with just this event being executed differently from the rest of the image editor, nor did I see it as worthwhile to refactor the entire image editor. So, I pretty much stand by that approach in this case, even though it's far from ideal.

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


5 months ago

#6 @joedolson
5 months ago

  • Milestone changed from Awaiting Review to 6.6

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


7 weeks ago

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


6 weeks ago

#9 @nirajgirixd
5 weeks ago

I haven't seen any PR raised for this issue. If it is still valid, shall I raise a PR for the fix?

cc: @afercia @joedolson

Last edited 5 weeks ago by nirajgirixd (previous) (diff)

#10 @afercia
5 weeks ago

@nirajgirixd sure please do feel free to go ahead, thanks!

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


5 weeks ago

#12 @sabernhardt
5 weeks ago

  • Keywords needs-patch added
  • Milestone changed from 6.6 to 6.7

This ticket was mentioned in PR #6856 on WordPress/wordpress-develop by @nirajgirixd.


5 weeks ago
#13

  • Keywords has-patch added; needs-patch removed

## Description

This PR addresses the scrolling issue encountered when using the keyboard to navigate different options in the image editor.

## Changes Made

  • Replaced onkeyup event with onkeydown event.

## Why?

When using the keyboard to navigate the items in the 'Image rotation' dropdown menu, the page scrolls.

## Screencast

https://github.com/WordPress/wordpress-develop/assets/51747980/fe1f8e04-0a3d-462a-8d10-00ca4ecdaae7

---

## Trac ticket: https://core.trac.wordpress.org/ticket/60548

Note: See TracTickets for help on using tickets.