Make WordPress Core

Opened 3 months ago

Closed 2 months 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 3 months ago.
Screenshot
54411.diff (465 bytes) - added by joedolson 3 months ago.
Change test to check whether upload button is replaced.

Download all attachments as: .zip

Change History (11)

#1 @SergeyBiryukov
3 months ago

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

#2 @talldanwp
3 months 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.

Version 1, edited 3 months ago by talldanwp (previous) (next) (diff)

#3 @joedolson
3 months ago

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

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

Change test to check whether upload button is replaced.

#5 @joedolson
3 months ago

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

#6 follow-up: @joedolson
3 months 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 months 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
2 months 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
2 months ago

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

Fixed by reverting [52059]

Note: See TracTickets for help on using tickets.