Make WordPress Core

Opened 4 months ago

Last modified 2 weeks ago

#60074 assigned defect (bug)

Add New Media File > The ''dismiss button ''not showing any response when uploading the unsupported (.jfif) file

Reported by: vivekawsm's profile vivekawsm Owned by: adamsilverstein's profile adamsilverstein
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 6.4.2
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Description

The dismiss button is not working when uploading the not allowed file type ( .jfif file)

Environment

  • WordPress: 6.4.2
  • PHP: 8.2.4
  • Server: Apache/2.4.56 (Win64) OpenSSL/1.1.1t PHP/8.2.4
  • Database: mysqli (Server: 10.4.28-MariaDB / Client: mysqlnd 8.2.4)
  • Browser: Chrome 119.0.0.0 (Windows 10/11)
  • Theme: Twenty Twenty-Four 1.0
  • Plugins:
    • WordPress Beta Tester 3.5.4

Steps to Reproduce

  1. Select the Add New Media File option from the Media menu
  2. Upload the .jfif file
  3. click the Dismiss option
  4. No response showing when clicked

Expected Results

The dismiss pop-up should be disable when clicking on the Dismiss button

Actual Results

No response was showing when clicked on the Dismiss button

Video attached :
https://www.loom.com/share/9130cd6cc58b48a8aae66849dc11670f

Attachments (5)

dwsample-jfif-640.jfif (49.1 KB) - added by vivekawsm 4 months ago.
Uploaded (.jfif) file
Screenshot 2023-12-14 224652.png (149.5 KB) - added by vivekawsm 4 months ago.
Attachment of Dismiss button not working
60074.diff (946 bytes) - added by mijotj 4 months ago.
Patch added
60074.2.diff (1.5 KB) - added by mijotj 4 months ago.
New patch added
60074.3.diff (874 bytes) - added by adamsilverstein 4 months ago.

Download all attachments as: .zip

Change History (19)

@vivekawsm
4 months ago

Uploaded (.jfif) file

@vivekawsm
4 months ago

Attachment of Dismiss button not working

#1 @adhun
4 months ago

i was able to reproduce this issue.

Environment

  • WordPress: 6.4.2
  • PHP: 7.4.21
  • Server: Apache/2.4.46 (Unix) OpenSSL/1.0.2u PHP/7.4.21 mod_wsgi/3.5 Python/2.7.18 mod_fastcgi/mod_fastcgi-SNAP-0910052141 mod_perl/2.0.11 Perl/v5.30.1
  • Database: mysqli (Server: 5.7.34 / Client: mysqlnd 7.4.21)
  • Browser: Chrome 119.0.0.0 (macOS)
  • Theme: Twenty Twenty-Three 1.3
  • MU-Plugins: None activated
  • Plugins:
    • WordPress Beta Tester 3.5.2

@mijotj
4 months ago

Patch added

#2 @adamsilverstein
4 months ago

Hey @mijotj - thanks for the patch - I will give it a test.

I noticed your patch actually removes some inline JavaScript and I'm curious why do you think that fixes the issue? It might be worth seeing why that inline JS was added originally.

#3 @sarath.ar
4 months ago

I tested the patch, and it appears that the issue is resolved.

Environment

  • WordPress: 6.4.2
  • PHP: 7.4.21
  • Server: Apache/2.4.46 (Unix) OpenSSL/1.0.2u PHP/7.4.21 mod_wsgi/3.5 Python/2.7.18 mod_fastcgi/mod_fastcgi-SNAP-0910052141 mod_perl/2.0.11 Perl/v5.30.1
  • Database: mysqli (Server: 5.7.34 / Client: mysqlnd 7.4.21)
  • Browser: Firefox 120.0 (macOS)
  • Theme: Twenty Twenty-Three 1.3
  • MU-Plugins: None activated
  • Plugins:
    • WordPress Beta Tester 3.5.5

#4 follow-up: @adamsilverstein
4 months ago

Although the current patch fixes the issue, the button is being changed to a link and I found that it was previously changed from a link to a button for accessibility reasons, see https://github.com/WordPress/wordpress-develop/commit/ef84e589d2bc25952bee31940a5a11c3fa75f599

For this reason, I don't think we should make the fix as proposed, can we find a different fix that maintains the semantic button element?

I agree we should remove the inline JavaScript anyway I believe it is being stripped already. Can we expand the existing JS handler to correctly handle the button click?

@mijotj
4 months ago

New patch added

#5 in reply to: ↑ 4 @mijotj
4 months ago

Replying to adamsilverstein:
Thank you for testing my patch. I am uploading a new patch as per your suggestion.

#6 @sarath.ar
4 months ago

  • Keywords has-patch added; needs-patch removed

I tested the new patch as well, and it appears that the issue is resolved.

Before applying the patch: https://www.loom.com/share/e5e58cef24ce49049aacd66bdd3fdf76
After applying the patch: https://www.loom.com/share/04a0850967354c4691ebbc49bdfe1b53

Environment

  • WordPress: 6.4.2
  • PHP: 7.4.21
  • Server: Apache/2.4.46 (Unix) OpenSSL/1.0.2u PHP/7.4.21 mod_wsgi/3.5 Python/2.7.18 mod_fastcgi/mod_fastcgi-SNAP-0910052141 mod_perl/2.0.11 Perl/v5.30.1
  • Database: mysqli (Server: 5.7.34 / Client: mysqlnd 7.4.21)
  • Browser: Firefox 120.0 (macOS)
  • Theme: Twenty Twenty-Three 1.3
  • MU-Plugins: None activated
  • Plugins:
    • WordPress Beta Tester 3.5.5

#7 @adamsilverstein
4 months ago

  • Keywords needs-patch added; has-patch removed

@mijotj - that is much closer and what I though about trying at first as well. Unfortunately the plupload code (and everything in that vendor file) are upstream dependencies - meaning they are pulled in during our build file and we can't edit them directly - the changes would be overwritten during the build.

I guess because we can't edit that file, we can't depend on it closing the error. Therefore we need to put the handler somewhere else. since we only use this when an error shows and its a tiny snippet, inlining it seems appropriate.

The current code seems to be stripping the html added to the tag, maybe we can just add it immediately after the tag?

#8 follow-up: @adamsilverstein
4 months ago

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

60074.3.diff is a start, however the selector may be too broad?

Last edited 4 months ago by adamsilverstein (previous) (diff)

#9 @adamsilverstein
4 months ago

On a separate note, should we consider adding support for uploading ".jfif" files (so you don't get the error in the first place)? do all the browsers support them? what about GD or Imagick. First time I have seen this format used, does it offer specific advantages?

#10 @mijotj
4 months ago

@adamsilverstein , this issue isn't specific to ".jfif". It happens whenever someone tries to upload unsupported files. I'm trying to fix it.

#11 @adamsilverstein
4 months ago

@adamsilverstein , this issue isn't specific to ".jfif". It happens whenever someone tries to upload unsupported files. I'm trying to fix it.

Yes, I get that we need to fix the dismiss notice not closing. I was wondering separately about the jfif format and whether we should support it (this would require a separate ticket).

#12 in reply to: ↑ 8 @mijotj
4 months ago

Replying to adamsilverstein:
I tested this patch as well, and it is working very well.

This ticket was mentioned in PR #5772 on WordPress/wordpress-develop by @adamsilverstein.


4 months ago
#13

  • Keywords has-patch added; needs-patch removed

Trac ticket:

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


2 weeks ago

Note: See TracTickets for help on using tickets.