Make WordPress Core

Opened 13 months ago

Closed 3 weeks ago

#39859 closed defect (bug) (fixed)

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

Reported by: andreiglingeanu Owned by: afercia
Milestone: 4.9.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch fixed-major
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 (6)

patch.diff (431 bytes) - added by andreiglingeanu 13 months ago.
patch.2.diff (1.3 KB) - added by andreiglingeanu 4 months ago.
39859.diff (3.8 KB) - added by afercia 3 months ago.
39859.2.diff (4.1 KB) - added by afercia 6 weeks ago.
39859.3.diff (5.1 KB) - added by afercia 6 weeks ago.
39859.4.diff (5.6 KB) - added by adamsilverstein 5 weeks ago.

Download all attachments as: .zip

Change History (38)

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

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

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

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

10 months ago

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

  • Focuses accessibility added; ui administration removed

#6 @afercia
7 months ago

#39460 was marked as a duplicate.

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

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

#9 @andreiglingeanu
4 months 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 4 months ago by andreiglingeanu (previous) (diff)

#10 @andreiglingeanu
3 months ago

Anything new on that one?

#11 @swissspidy
3 months ago

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

Shamelessly adding Andrea as the owner here for review.

3 months ago

#12 @afercia
3 months 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
3 months 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.

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

2 months ago

#15 @afercia
8 weeks ago

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.

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

7 weeks ago

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

6 weeks ago

6 weeks ago

#18 @afercia
6 weeks ago

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

6 weeks ago

#19 @afercia
6 weeks ago

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

#20 @dd32
6 weeks ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

#21 @afercia
5 weeks ago

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)

#22 @adamsilverstein
5 weeks ago

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

#23 @adamsilverstein
5 weeks ago

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?

#24 @afercia
5 weeks ago

  • Keywords needs-refresh added

@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"?

#25 @adamsilverstein
5 weeks ago

  • Keywords commit added; needs-refresh removed

@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!

#26 @afercia
5 weeks ago

@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

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

4 weeks ago

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

4 weeks ago

#29 @afercia
4 weeks ago

  • Keywords commit removed
  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in [42624].

#30 @afercia
4 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

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

3 weeks ago

#32 @SergeyBiryukov
3 weeks ago

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

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.

Note: See TracTickets for help on using tickets.