WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

#54411 closed defect (bug) (fixed)

Media Library modal - multiple Select Files buttons when switching between Media Library and Upload Files tabs

Reported by: talldanwp Owned by: joedolson
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

When switching between the Media Library and Upload Files tabs in the media modal, the Select Files button ends up being duplicated many times.

Steps to reproduce

  1. Create a post in the classic editor or block editor
  2. Add media, in the block editor add an image block and select 'Upload'
  3. Switch between the Media Library and Upload Files tabs in the modal several times
  4. Observe that each time you switch tabs there are more 'Select Files' buttons appended.

Expected: Only one Select File button.

I tested and confirmed this seems to have been introduced in 5.9, it wasn't an issue in 5.8.1.

The bug happens in both classic and block editors, with or without Gutenberg active, so definitely seems like a core issue.

Attachments (2)

Screenshot 2021-11-10 at 12.23.59 pm.png (168.8 KB) - added by talldanwp 4 weeks ago.
Screenshot
54411.diff (465 bytes) - added by joedolson 3 weeks ago.
Change test to check whether upload button is replaced.

Download all attachments as: .zip

Change History (11)

#1 @SergeyBiryukov
4 weeks ago

Appears to be introduced in [52059] / #54168.

#2 @talldanwp
4 weeks ago

I'm not actually sure that change in [52059] was needed for e2e tests. Gutenberg e2e tests have always targeted a hidden input with the file type when interacting with the media modal.

There was an existing input in the media modal in an element with the class name 'moxie-shim'.

A revert of that change might be possible here, but that would be dependent on whether end to end tests that depend on the change have already been committed.

edit: just an additional note, [52059] was actually causing Gutenberg tests to fail, I had some difficulty getting this new input to work in e2e tests there (https://github.com/WordPress/gutenberg/pull/36368#issuecomment-964915100).

Last edited 4 weeks ago by talldanwp (previous) (diff)

#3 @joedolson
3 weeks ago

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

#4 @joedolson
3 weeks ago

Because the passed browse button is now a container div rather than a single element, we're getting a mismatch between states - the div contains whitespace. (It also had style attributes added by moxie, but while that causes a failure, it's easy to work around.)

Fixed this by changing the check to verify that the placeholder has content; if it does, then the replacement has already happened.

@joedolson
3 weeks ago

Change test to check whether upload button is replaced.

#5 @joedolson
3 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed

#6 follow-up: @joedolson
3 weeks ago

There are still problems with keyboard focus in the media modal & mouse operability. I'm dubious about whether this change is worth the effort; would love to hear further explanation from @justinahinon about what benefits #54168 offers for e2e tests; otherwise I think the resolution might be to revert 54168.

#7 in reply to: ↑ 6 @justinahinon
3 weeks ago

Replying to joedolson:

There are still problems with keyboard focus in the media modal & mouse operability. I'm dubious about whether this change is worth the effort; would love to hear further explanation from @justinahinon about what benefits #54168 offers for e2e tests; otherwise I think the resolution might be to revert 54168.

Hello @joedolson. I've made a couple of tests with WP before [52059]. As @talldanwp mentioned, it is possible to make uploads in e2e using the hidden file type input .moxie-shim input. This is my mad for not make extensive researches.

I think [52059] can safely be reverted.

#8 @joedolson
3 weeks ago

In 52171:

Media: Revert media uploader input change in [52059].

Based on follow-up research, this change was never necessary in order to use e2e tests in the media library uploader. Additionally, it created several complicated side effects. Without significant benefit, it's not valuable to pursue the change further.

Follow up to [52059].

See #54168, #54411.
Fixes #54168.

#9 @joedolson
3 weeks ago

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

Fixed by reverting [52059]

Note: See TracTickets for help on using tickets.