Make WordPress Core

Opened 10 years ago

Last modified 4 weeks ago

#37669 new defect (bug)

Shiny Updates: Cancelling the filesystem credentials when updating within the pop-up prevents updating again (without closing/opening) and leaves UI artefacts

Reported by: DavidAnderson Owned by:
Priority: normal Milestone: Future Release
Component: Themes Version: 4.6
Severity: normal Keywords: good-first-bug has-patch has-unit-tests
Cc: Focuses: javascript, administration

Description (last modified by ocean90)

Reproducible on RC3: 4.6-RC3-38260

Reproducible: every time

Steps to reproduce:

1) Set up WordPress, and set permissions to that the 'filesystem credentials' dialog will be required when performing theme updates.

2) Tweak a version number so that you will have a theme that needs an update. Then, go to /wp-admin/themes.php

3) Do *not* click on the immediately visible "Update now". Instead, click on the theme such that the pop-up pops up.

4) Click on the "Update now" link within the pop-up. If step 1) was done correctly, then the filesystem dialog should now pop up.

5) Close the filesystem credentials dialog.

Result: "Updating..." will still show there. But nothing will be updating. You cannot update until you close the pop-up and re-open it.

Excepted result: The "update now" link re-appears, and "Updating.." disappears.

Result: nothing happens.

#37285 is similar, but a separate (and now fixed) issue.

Attachments (3)

37669.gif (721.6 KB ) - added by ocean90 10 years ago.
37669.diff (3.5 KB ) - added by fgiannar 7 years ago.
37669.2.diff (3.5 KB ) - added by fgiannar 7 years ago.

Download all attachments as: .zip

Change History (16)

@ocean90
10 years ago

#1 @ocean90
10 years ago

  • Component Upgrade/InstallThemes
  • Description modified (diff)
  • Focuses administration added
  • Keywords needs-patch added
  • Summary Shiny updates bug (WP 4.6 RC 3): Cancelling the filesystem credentials when updating within the pop-up prevents updating again (without closing/opening) and leaves UI artefactsShiny Updates: Cancelling the filesystem credentials when updating within the pop-up prevents updating again (without closing/opening) and leaves UI artefacts

Confirmed, see 37669.gif.

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


10 years ago

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


10 years ago

#4 @ocean90
10 years ago

  • Milestone Awaiting ReviewFuture Release

#5 @desrosj
7 years ago

  • Keywords good-first-bug added
  • MilestoneFuture Release

@fgiannar
7 years ago

#6 @fgiannar
7 years ago

  • Keywords has-patch added; needs-patch removed

Hi there,

@desrosj This is an attempt to fix this issue. Please check it out and let me know accordingly.
Note that I checked out the corresponding qunit tests ( tests/qunit/wp-admin/js/updates.js ) but based on the existing content I was unsure if I needed to add a new one for this case.

Thanks,

@fgiannar
7 years ago

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


6 years ago

#8 @Laxman Prajapati
6 years ago

Can you please try to add this line

define('FS_METHOD','direct');

In root wp-config.php file and after that try again please.

Thanks

#9 @DavidAnderson
6 years ago

This is not the place for support advice. The point is not to hide the bug on individual sites via not using the affected feature, it's to fix it.

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


4 months ago
#10

  • Keywords has-unit-tests added

## Summary

  • restore the theme details popup notice when the filesystem credentials modal is canceled
  • add a QUnit regression test for the queued theme update cancel flow

## Testing

  • vendor/bin/phpcbf --standard=phpcs.xml.dist src/js/_enqueues/wp/updates.js tests/qunit/wp-admin/js/updates.js
  • vendor/bin/phpcs --standard=phpcs.xml.dist src/js/_enqueues/wp/updates.js tests/qunit/wp-admin/js/updates.js
  • npx tsc --build
  • npx playwright test --config tests/qunit/playwright.config.js

rtCamp issue: https://github.com/rtCamp/wp-contributions/issues/3067

#11 @369work
2 months ago

I first tested the original 37669.2.diff.
After noticing this newer PR,
I removed the earlier patch and re-tested using this PR on its own.

Test setup

  • WordPress 7.0.1, PHP 8.4.23, single site
  • Forced the credentials modal by setting define( 'FS_METHOD', 'ftpext' );
  • SCRIPT_DEBUG on
  • Theme: Twenty Twenty-Three, installed at 1.3 with 1.6 available
  • Trigger: the "update now" link inside the theme details modal

(Appearance > Themes > click the theme thumbnail), not the theme grid card

Testing revealed two separate issues.

1. The stuck spinner

With only this PR applied, the spinner still does not stop on WordPress7.0.1.

After clicking "update now" in the theme details modal and then Cancel on
the Connection Information dialog, the "Updating..." spinner remains
stuck.

I then restored the corresponding changes from 37669.2.diff and
re-tested.
With those changes restored, the spinner stopped correctly.

Console after Cancel:

  • jQuery(".updating-message").length → 1 (one notice not reset)
  • The remaining element has the class "notice notice-warning notice-alt updating-message", and .closest("[data-slug]") returns undefined, indicating that the remaining notice is the overlay (modal) notice rather than the theme grid card.

The new

$message.add( $( '#update-theme' ).closest( '.notice' ) )

does resolve to a valid element.

However, the cancel handler still calls

$message.hasClass( 'updating-message' )

which only checks the first element of the jQuery collection, so the
overlay notice keeps the class.

In my testing, the spinner only stops when both parts of
37669.2.diff are also present:

(a) the themes.php markup change adding data-slug and
update-message to the overlay notice, allowing the standard
[data-slug] .update-message selector to find it; and

(b) iterating the matched elements with .each() instead of treating
$message as a single element.

With those two changes added, this PR stops the spinner reliably.

2. Clicking "update now" again

While investigating this, I also noticed a second issue.

Even when the spinner is fixed, clicking "update now" a second time
within the still-open modal does nothing.

The restore step uses

.html( originalText )

which replaces the original link markup.

After Cancel, the control becomes

<button class="button-link" type="button">Update now</button>

instead of the original

<a id="update-theme" data-slug="...">

As a result, the click handler bound via #update-theme no longer
matches, and the update cannot be retried without reloading the page.

The console confirms that

jQuery("#update-theme").length

returns 0 after Cancel.

wp.updates.ajaxLocked is correctly false at this point, so the lock
does not appear to be the cause.
The problem appears to be that the restore step removes the original link element and its event binding.

I'd be happy to test any follow-up changes.

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


8 weeks ago
#12

Canceling the filesystem-credentials modal after clicking "update now" inside the theme details overlay (Appearance → Themes → click a theme's screenshot → "update now") left the overlay's "Updating…" spinner stuck forever and broke the retry click, even though the same action from the theme grid card's own update link reset correctly. This was caused by two compounding bugs in wp.updates: the overlay's notice had no data-slug, so the cancel handler's selector never reached it at all; and even once reachable, updateTheme() stored each notice's original text via $notice.data('originaltext', $notice.html()) on a combined multi-element jQuery collection, where both .data() and .html() only operate on the first element — so the overlay and the grid card ended up sharing one snapshot, and canceling could paste one notice's markup into the other instead of restoring each one's own content.

This adds a data-slug attribute to the theme details overlay root in Details.render() (mirroring the existing convention already used by the grid card view) and marks the overlay's update notice with the same update-message class the grid card uses, so both notices are reachable through one selector. updateTheme() and the credential-modal-cancel handler in updates.js now iterate matched notices with .each() so each one's original text is read and restored independently instead of relying on jQuery's first-element-only .data()/.html() behavior across a multi-element collection.

Verified with a browser harness driving the real compiled wp.updates script through the exact scenario (open overlay → update → credentials required → cancel), confirming it reproduces the reported symptom on unfixed code and is resolved after the fix, plus manual verification in a running wp-env with a forced FS_METHOD and a faked theme update.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Sonnet 5
Used for: Root cause analysis, implementation, test coverage, and manual verification. Reviewed and directed by me throughout.

#13 @arkaprabhachowdhury
4 weeks ago

Opened [PR #12033](https://github.com/WordPress/wordpress-develop/pull/12033) to address this ticket. Summary - restore the theme details popup notice when the filesystem credentials modal is canceled - add a QUnit regression test for the queued theme update cancel flow Testing - vendor/bin/phpcbf --standard=phpcs.xml.dist src/js/_enqueues/wp/updates.js tests/qunit/wp-admin/js/updates.js - vendor/bin/phpcs --standard=phpcs.xml.dist src/js/_enqueues/wp/updates.js tests/qunit/wp-admin/js/updates.js - npx tsc --build - npx playwright test --config tests/qunit/playwright.config.js The PR includes focused regression coverage, and its validation details are recorded in the PR description.

I picked this up because the ticket describes a concrete core behavior or edge case that remains relevant in current WordPress. The proposed change is intentionally scoped to the ticket and is submitted here for code review, with the technical discussion remaining on this ticket.

Note: See TracTickets for help on using tickets.