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

[andreiglingeanu]

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.

[swissspidy]

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

[andreiglingeanu]

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

[afercia]

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"

[afercia]

#39460 was marked as a duplicate.

[andreiglingeanu]

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.

[afercia]

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

[andreiglingeanu]

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.

2. 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.

[andreiglingeanu]

Anything new on that one?

[swissspidy]

Shamelessly adding Andrea as the owner here for review.

[afercia]

@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:
https://core.trac.wordpress.org/changeset/23029
https://core.trac.wordpress.org/ticket/22716

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.

[andreiglingeanu]

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.

[afercia]

can you provide me the reproduction steps for that

Sorry for late reply. I meant without the patch applied:

• run grunt watch
• edit src/wp-includes/js/media/views/modal.js and set freeze to false
• in Firefox Quantum, edit a post and click Add Media
• the modal opens
• close the modal
• the page is scrolled to the bottom

This just to double check that freeze was still needed for Firefox but now is not necessary any longer, since focus is moved to a more proper element.

[afercia]

39859.2.diff​ removes an unused options to make JSHint happy.

[afercia]

In 39859.3.diff​ I think using find() is unnecessary and can be avoided.

[dd32]

Bumping to 4.9.3 due to 4.9.2s release

[afercia]

Just two things I'd like to hear some thoughts from @adamsilverstein :) thanks!

• is there any better way to target that element other than doing this.$( '.media-modal' ).focus(); ?
• should the removal of the freeze option be mentioned in a dev-notes post? (I'd say no since it was used internally, but worth considering)

[adamsilverstein]

@afercia in my testing focusing this.$el seemed to work, without causing any scrolling?

if that is still causing the scrolling, I think the approach with find is better (this.$el.find( '.media-modal' ).focus(); ) - I'm concerned that this.$( '.media-modal' ) might not find the right modal if two were open at the same time? I think this.$el refers to the specific modal instance that is being opened, while this.$ refers to the (global) jQuery object.

[adamsilverstein]

39859.4.diff​ leaves the focus call unchanged, in my testing I did not see scrolling - I tested in the latest versions of firefox and chrome. am I missing a testing step?

[afercia]

@adamsilverstein the whole point of this ticket is to change the element that receive focus :) latest patch defeats that purpose. as per the second point, I though this.$ meant in the context of "this view"?

[adamsilverstein]

@afercia aha, i missed the point! I thought the main issue was the scrolling - I see now that is just a side effect of the focus problem.

On the second point you are exactly right, my recollection was incorrect... Checking the docs ​http://backbonejs.org/#View-dollar it explicitly states that {view}.$ can be used as a scoped selector: It's equivalent to running: view.$el.find(selector). Apologies for misspeaking earlier about that.

All that said, I think 39859.3.diff​ is good to go!

[afercia]

@adamsilverstein thanks very much! Actually, a screenshot could have helped :) For reference and history, here it is:

I've made the dark overlay a bit more transparent for testing purposes: the page behind the modal still scrolls to the bottom and displays the footer. This is more evident when plugins use custom modals that take less space on the screen, as in the original video linked by @andreiglingeanu see ​https://www.screenmailer.com/v/o201KLQ9pKOvBaU

[afercia]

Fixed in [42624].

[afercia]

Reopening for 4.9.3 consideration.

Worth noting this change removes the freeze option from the media modal view. Though minor, if this change needs a dev note than I'd suggest to not merge it in 4.9.3.

[SergeyBiryukov]

In 42627:

Media: avoid page scrolling when opening the media modal.

Moves focus to a proper element within the media modal to avoid the underlying
page to scroll to the bottom. Removes the media modal freeze option.

Props andreiglingeanu, adamsilverstein.
Merges [42624] to the 4.9 branch.
Fixes #39859.

[andreiglingeanu]

Finally removing plugin-land hacks & monkey patches! Thank you!
​https://github.com/ThemeFuse/Unyson/pull/3221/files