Make WordPress Core

Opened 17 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#60074 closed defect (bug) (fixed)

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: joedolson's profile joedolson
Milestone: 6.8 Priority: normal
Severity: normal Version: 6.4.2
Component: Media Keywords: has-patch has-testing-info commit
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 (7)

dwsample-jfif-640.jfif (49.1 KB) - added by vivekawsm 17 months ago.
Uploaded (.jfif) file
Screenshot 2023-12-14 224652.png (149.5 KB) - added by vivekawsm 17 months ago.
Attachment of Dismiss button not working
60074.diff (946 bytes) - added by mijotj 17 months ago.
Patch added
60074.2.diff (1.5 KB) - added by mijotj 17 months ago.
New patch added
60074.3.diff (874 bytes) - added by adamsilverstein 17 months ago.
5772-before.mp4 (57.1 KB) - added by parthvataliya 5 months ago.
before patch
5772-after.mp4 (57.8 KB) - added by parthvataliya 5 months ago.
After patch

Download all attachments as: .zip

Change History (47)

@vivekawsm
17 months ago

Uploaded (.jfif) file

@vivekawsm
17 months ago

Attachment of Dismiss button not working

#1 @adhun
17 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
17 months ago

Patch added

#2 @adamsilverstein
17 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
17 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
17 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
17 months ago

New patch added

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

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

#6 @sarath.ar
17 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
17 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
17 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 17 months ago by adamsilverstein (previous) (diff)

#9 @adamsilverstein
17 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
17 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
17 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
17 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.


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


13 months ago

#15 follow-up: @joedolson
10 months ago

  • Milestone changed from Awaiting Review to 6.7

Milestoning this for 6.7. If you don't have time for it @adamsilverstein, I can take ownership.

#16 in reply to: ↑ 15 @adamsilverstein
10 months ago

Replying to joedolson:

Milestoning this for 6.7. If you don't have time for it @adamsilverstein, I can take ownership.

Feel free to take it @joedolson and let me know I can assist.

Thanks!

#17 @joedolson
10 months ago

  • Owner changed from adamsilverstein to joedolson
  • Status changed from assigned to accepted

#18 @peterwilsoncc
7 months ago

  • Keywords changes-requested added

I've added a minor change request to the associated pull request to avoid multiple events.

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


7 months ago

#20 @chaion07
7 months ago

  • Keywords needs-testing added

Thanks @vivekawsm for reporting this. We reviewed this Ticket during a recent bug-scrub session. Here's a summary of the discusison:

  1. The patch needs testing so we are adding the needs-testing keyword
  2. The dismiss button is working in this /wp-admin/upload.php URL but not wp-admin/media-new.php

Thank you.

Props to @sayedulsayem

Cheers!

#21 @ugyensupport
7 months ago

I have updated the assurance with given latest patch https://core.trac.wordpress.org/attachment/ticket/60074/60074.3.diff,
I don't see any fixes.

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


7 months ago

#23 @stoyangeorgiev
7 months ago

  • Milestone changed from 6.7 to 6.8

This one was discussed at a bug-scrub session. At the time of the discussion this one still needed testing. Beta 3 was also couple of hours away, so we are moving this to 6.8

#24 @sppramodh
6 months ago

  • Keywords has-testing-info added

Reproduction Report

Description

This report validates whether the issue can be reproduced.

Environment

  • WordPress: 6.7-RC2
  • PHP: 8.2.18
  • Server: Apache/2.4.59 (Win64) PHP/8.2.18 mod_fcgid/2.3.10-dev
  • Database: mysqli (Server: 8.3.0 / Client: mysqlnd 8.2.18)
  • Browser: Firefox 132.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Four 1.2
  • MU Plugins: None activated
  • Plugins:
    • Secure Custom Fields 6.3.10.2
    • Test Reports 1.1.0
    • WooCommerce 9.3.3
    • WordPress Beta Tester 3.6.1

Actual Results

  1. ❌ Error condition dose not occurs (cannot be reproduced).

Additional Notes

  • Tested by uploading a file larger than the set maximum upload size.
  • Tested by uploading an unsupported file format (.jfif) attached in this ticket.
  • The "Dismiss" button has been moved; it is now displayed below the error message instead of in the top right corner.
  • Clicking the "Dismiss" button successfully removes the error message.
  • The issue appears to be resolved in the latest release; this ticket can be closed.

Supplemental Artifacts

Video : (https://www.loom.com/share/f2daf07a46cb4078b68f7e5d081a7b19?sid=5fa71435-2a23-4f8e-a8e8-e0ba98c4e100)?

This ticket was mentioned in Slack in #core-test by sppramodh. View the logs.


6 months ago

#26 @indirabiswas27
6 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5772

Environment:

OS: Windows 11
Browser: Chrome

Actual Result:
The Dismiss button is working fine. clicking it successfully removes the notice.

Screenshots:

Before: https://www.loom.com/share/fc9913423ee34f9ab9fe1a918b214566?sid=950c520e-77a2-44cf-a1d4-bbc6853debb1

After: https://www.loom.com/share/536a6e324901439ea87562831d20c31e?sid=8cbaa87a-f8f6-43ec-8a3e-6f51c0571ebf

#27 @parthvataliya
5 months ago

Test Report

Description

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

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5772

Environment

  • WordPress: 6.8-alpha-59274-src
  • PHP: 8.2.22
  • Server: nginx/1.27.0
  • Database: mysqli (Server: 8.0.39 / Client: mysqlnd 8.2.22)
  • Browser: Chrome 129.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty 2.8
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

@parthvataliya
5 months ago

before patch

@parthvataliya
5 months ago

After patch

#28 @aishwarryapande
5 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5772

Environment:
OS: Windows 11
Browser: Chrome
PHP: 8.3
WordPress: 6.7.1
Theme: Twenty Twenty Four

Result: The dismiss button is working properly.

Status : It is working fine

Screenshot

Before
https://drive.google.com/file/d/1YT7psSALfK5moDdMaFNLQDvSq5yYcjJ9/view?usp=sharing

After
https://drive.google.com/file/d/1g89ubKLtEhhOvlziQLICuHwQJ-PBkRDq/view?usp=sharing

Last edited 5 months ago by aishwarryapande (previous) (diff)

#29 @dhrumilk
4 months ago

Test Report

Description

I have tested the new patch, and I can confirm that the issue has been successfully resolved.

Patch Tested: https://github.com/WordPress/wordpress-develop/pull/5772

Environment:

WordPress Version: 6.8-alpha-59274-src
Server: nginx/1.27.0
PHP Version: 8.3
Browser: Chrome 131.0.6778.205
Operating System: Linux
Theme: Twenty Twenty 2.8

Actual Results

✅ The issue has been resolved with the patch.

#30 @manojmaharrshi
2 months ago

Test Report

Description

This report validates whether the patch for dismiss button works as expected

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5772.diff

Environment

  • WordPress: 6.8-alpha-59840
  • PHP: 8.1.29
  • Server: nginx/1.16.0
  • Database: mysqli (Server: 8.0.16 / Client: mysqlnd 8.1.29)
  • Browser: Chrome 133.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.3
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0
    • WordPress Beta Tester 3.6.2

Actual Results

  1. ✅ Issue resolved with patch.

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


2 months ago

#32 @ugyensupport
2 months ago

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

Description

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

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5772.diff

Environment

  • WordPress: 6.7.2
  • PHP: 8.4.1
  • Server: nginx/1.21.4
  • Database: mysqli (Server: 5.7.44-log / Client: mysqlnd 8.4.1)
  • Browser: Chrome 133.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.3
  • MU Plugins: None activated
  • Plugins:
    • Gutenberg 20.3.0
    • Test Reports 1.2.0
    • WordPress Beta Tester 3.6.2

Actual Results

  1. ✅ Issue resolved with patch.

#33 @imranhasanraaz
8 weeks ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5772.diff

Environment

  • WordPress: 6.8-beta1-59933-src
  • PHP: 7.4.33
  • Server: nginx/1.27.2
  • Database: mysqli (Server: 8.0.40 / Client: mysqlnd 7.4.33)
  • Browser: Chrome 133.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Four 1.3
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.1.0

Actual Results

✅ Issue resolved with patch.
Before : https://drive.google.com/file/d/12e8oWhKLAXdlAR9064nahaTWwG0XEJi3/view?usp=sharing
After : https://drive.google.com/file/d/1gA2X_d8ffcMCy4D9JdOizCt4zWyHsvAP/view?usp=sharing

#34 @peterwilsoncc
8 weeks ago

I updated @adamsilverstein's pull request as it has some E2E tests. I think it's now ready for a review.

The changes I made to the original code:

  • Modified the tests, the file needed to be renamed to get them to run. Then they needed a little work.
  • Modified the code to use an ID for each button, this was to avoid multiple events being added to each button as noted a while ago.
  • Merged in trunk because it had been a while.

I'm seeing some flakeyness with admin.visitAdminPage() failing to log the user in and causing the tests to fail on some runs but not others. I'll ask around but if they're going to throw false errors I'll skip them

#35 @pkbhatt
7 weeks ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5772

Environment:
OS: Ubuntu
Browser: Chrome
PHP: 8.3
WordPress: 6.7.2.beta-2
Theme: Twenty Twenty Four

Result: The dismiss button is working properly.

Status : It is working fine

Screenshot

Before: https://www.awesomescreenshot.com/video/37582692?key=24b79e83722922b145c4e6f9fc0ce04b

After
https://www.awesomescreenshot.com/video/37583366?key=29f5382a23854697fb126e701e812901

#36 @shailu25
7 weeks ago

Test Report

Patch Tested: https://github.com/WordPress/wordpress-develop/pull/5772

Environment:
WordPress - 6.8-beta2
OS - Windows
Browser - Firefox
Theme: Twenty Twenty Five
PHP - 8.2.12
Active Plugin: Beta Tester

Actual Results:

  • Issue Resolved With Patch.✅

Screenshots:

Before Patch: https://drive.google.com/file/d/1k7mmKqPwqKiJrt7sMp3-X1_RHX1UuqMs/view
After Patch: https://drive.google.com/file/d/1i1zcSStmvQ--9MOg3K9FA3MKn53_1Mxt/view

#37 @joedolson
6 weeks ago

  • Keywords commit added; changes-requested needs-testing removed

I'm inclined to commit this with the potentially flaky test; we can easily revert the test if it causes problems. Currently seems to be passing, and maybe more eyes on the failure will help identify a reason.

#38 @joedolson
6 weeks ago

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

In 59986:

Media: Dismiss button not functional on upload errors.

Change the onclick attribute to a separate inlined script in the error message and improve the event attachment behavior.

Props vivekawsm, mijotj, adamsilverstein, parthvataliya, adhun, sarathar, peterwilsoncc, sayedulsayem, chaion07, sppramodh, indirabiswas27, aishwarryapande, dhrumilk, manojmaharrshi, ugyensupport, imranhasanraaz, pkbhatt, shailu25, joedolson.
Fixes #60074.

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


6 weeks ago

#40 @peterwilsoncc
6 weeks ago

#62606 was marked as a duplicate.

Note: See TracTickets for help on using tickets.