Make WordPress Core

Opened 8 months ago

Last modified 3 months ago

#39859 new defect (bug)

wp.media.view.Modal jumps when you open it

Reported by: andreiglingeanu Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: accessibility, javascript Cc:


Hello all,

To be short, this pull request fixes the jumping on the opening of a wp.media modal (or any modal that inherits it).

That is, this problem affects every modal in the admin area, just with the wp.media default size (which is almost full screen) you can't see that the body jumps.

fw.Modal (the modal that I'm opening in the video below) inherits from wp.media.view.MediaFrame, which in turn inherits from wp.media.view.Modal.

[BEFORE](https://www.screenmailer.com/v/o201KLQ9pKOvBaU) | [AFTER](https://www.screenmailer.com/v/DgUzmLbJQYaF3c8)

A bit more involved description of that change:

When you open any wp.media modal, you are calling wp.media.view.Modal.open method, which does a strange focus on its $el. [source](https://github.com/WordPress/WordPress/blob/4ca4ff999aba05892c05d26cddf3af540f47b93b/wp-includes/js/media-views.js#L6800)

It turns out, that this incorrect way to focus an element causes the whole page to change its scroll position.

Someone even introduced the [_freeze](https://github.com/WordPress/WordPress/blob/4ca4ff999aba05892c05d26cddf3af540f47b93b/wp-includes/js/media-views.js#L6777) option in order to prevent that bug, but this only causes the user to see more jumps: one at the modal opening and another one when you close the modal - see the video.

Disclaimer: I already did that change in a plugin that uses wp.media.view.Modal but I did it in a hacky way and I'd like it to be in core so that everyone can benefit from it.

Now I have two questions:

  1. This patch makes the focus to perform in a safe way, but I'm not deleting the _freeze option for the wp.media.view.Modal. Do I need to also remove that option and thus make the code a bit simpler?
  2. Do I need to compile my changes to the src/wp-includes/js/media/views/modal.js and send the new src/wp-includes/js/media-views.js file as part of my patch? Or having the src/wp-includes/js/media/views/modal.js changed is enough? The wp build process is aware about having to run grunt build?

Thank you for reviewing this.

Attachments (1)

patch.diff (431 bytes) - added by andreiglingeanu 8 months ago.

Download all attachments as: .zip

Change History (7)

#1 @swissspidy
8 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release

Ha, I never noticed that the page scrolls behind the modal when opening the media modal. Probably because it fills almost the whole page.

#2 @andreiglingeanu
8 months ago

@swissspidy Yeah, that's why you don't notice it with wp.media. You're simply not seeing it :)

Last edited 8 months ago by andreiglingeanu (previous) (diff)

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

6 months ago

#4 @afercia
3 months ago

It turns out, that this incorrect way to focus an element causes the whole page to change its scroll position.

Setting focus on a modal dialog is required, otherwise focus will stay on the page behind the modal thus making the modal not usable (or hardly usable) with a keyboard.

As far as I see, the real problem here is that focus is set on the modal wrapper: the element with id
id="__wp-uploader-id-{number here}"
this element has zero height and it's placed at the bottom of the source. The browser just scrolls to the focused element.

Ideally, focus should be set on the real modal element: the first child of id="__wp-uploader-id-{number here}"

  • should have tabindex="0"
  • should have an aria-labelledby attribute pointing to the visible heading in the modal
  • the visible heading in the modal should have an ID for that
  • since this is a modal with content that is not exclusively interactive, it should not have a role="dialog"

#5 @afercia
3 months ago

  • Focuses accessibility added; ui administration removed

#6 @afercia
3 months ago

#39460 was marked as a duplicate.

Note: See TracTickets for help on using tickets.