Make WordPress Core

Opened 10 months ago

Last modified 2 weeks ago

#39859 assigned defect (bug)

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

Reported by: andreiglingeanu Owned by: afercia
Milestone: 4.9.2 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 (3)

patch.diff (431 bytes) - added by andreiglingeanu 10 months ago.
patch.2.diff (1.3 KB) - added by andreiglingeanu 5 weeks ago.
39859.diff (3.8 KB) - added by afercia 2 weeks ago.

Download all attachments as: .zip

Change History (16)

#1 @swissspidy
10 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
10 months ago

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

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

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

8 months ago

#4 @afercia
5 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
5 months ago

  • Focuses accessibility added; ui administration removed

#6 @afercia
4 months ago

#39460 was marked as a duplicate.

#7 @andreiglingeanu
7 weeks ago

Hey @afercia thanks for your input on that and sorry for a very late reply.

I'll see what I can do here in order to implement your required changes. Do you understand the fact that this will affect every modal from wp.media.*? Is that something I should be proceeding to work on? I'm okay doing it if I receive a green light.

#8 @afercia
7 weeks ago

@andreiglingeanu I understand :) Do feel free to go ahead.

#9 @andreiglingeanu
5 weeks ago

Hey @afercia,

I'm getting back with a fresh patch on that which fixes that jump by swapping the element where the focus is happening on.

Everything works as expected but I do have two questions:

  1. I'm not really able to easily add the aria-labelledby attribute that will point to the visible heading in the modal. That's because sometimes, this blueprint Backbone View is used as part of other modals, which may or may not have a heading. That is, I would need to introduce a conditional somehow in the media-modal underscore template for deciding whether to insert or not that attribute. Is that something that really needs to be done? See this third-party plugin where this widget is used as part of another UI element.
  1. I'm getting back to the _freeze option for the wp.media.view.Modal, which I described back in the original description. Is that something I should remove as part of resolving that ticket? I feel like that was a hack around this wrongly focused element which caused the browser to jump back and forth.
Last edited 5 weeks ago by andreiglingeanu (previous) (diff)

#10 @andreiglingeanu
4 weeks ago

Anything new on that one?

#11 @swissspidy
3 weeks ago

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

Shamelessly adding Andrea as the owner here for review.

2 weeks ago

#12 @afercia
2 weeks ago

  • Milestone changed from Future Release to 4.9.2

@andreiglingeanu right, haven't found an easy way to detect when the modal has a title or not. Labeling a modal is very important for accessibility and can be done with either aria-labelledby targeting a visible element or with aria-label providing a value. However, it's out of the scope of this ticket so let's just fix the scrolling.

About freeze, yes seems it was introduced to fix the scrolling issue, see the relevant changeset and ticket:

I still can reproduce the scrolling bug in Firefox when I set freeze to false but avoiding the page to scroll in the first place makes the freeze workaround serve no purpose. I'd agree to remove it entirely.

Would you mind testing a bit 39859.diff on different browsers and in different parts of the admin (edit post, media grid, customizer)? Note: changes to the media js files must be applied while running grunt watch so the built files get automatically updated too.

#13 @andreiglingeanu
2 weeks ago

Just tried your patch on my end, everything looking good. And by the way, Firefox is not jumping at all for me (at least in the new FF Quantum), can you provide me the reproduction steps for that?

Very happy to see the freeze being dropped!

Thanks for reviewing & finishing that.

Note: See TracTickets for help on using tickets.