Double clicking the Update plugins-button gives... interesting experience!

[bitnissen]

Hi there,

I just updated to 4.8 on a bunch of sites and in one site I accidentally clicked the "Update"-button multiple times. What then happened, was a rather weird experience: After the update of the plugins successfully completed, it did another run where all updates apparently failed.

I tried this on multiple 4.8-installs, all has the same behaviour, so I guess it's fairly easy to reproduce. I've also recorded a video:

​http://d.pr/v/qmoc7E/5MFtrVpY.mp4

Steps to reproduce:

• Upgrade to 4.8.
• Go to "Plugins" » "Updates available" (you need at least a plugin that has updates ready for it)
• Select all update-ready plugins.
• Click the "Update"-button. Then click it again.
• Observe the table of plugins :-)

Kind regards,
Morten

[swissspidy]

Hey there,

Welcome to WordPress Trac and thanks for your report!

I'm pretty sure this isn't new in WordPress 4.8. This functionality was introduced in 4.6 with Shiny Updates. We've previously fixed this bug in the feature plugin (see ​https://github.com/obenland/shiny-updates/issues/145), but I guess not every instance of it.

[MarcGuay]

Because the Apply button is a submit input and not an anchor like in the other case, the solution will be a bit different. It seems that it will need the disabled property added somewhere in the click event in wp-admin/js/updates.js starting at line 1999, and then removed once the queue is finished working, but it's not obvious to me at the moment, it's my first time looking at this code.

[MarcGuay]

Remove extra line

[xkon]

On Update only and re enable on all notices just in case.

[xkon]

For some weird reason ( just in case it was working for you ) @MarcGuay s code was not disabling the button at all on my installation.

Either way I placed the code a little differently on ( 40966.3.diff ). It will disable the Apply button only in case of 'update-selected' and re-enable it either way whenever there's a notice. I think it's safer this way and at least it works for my installation.

Best regards,
Konstantinos

[afercia]

Please don't set disabled on focused elements because this will trigger a focus loss when using a keyboard. ideally, to avoid double clicks, the button should just no-op. /cc @swissspidy

[xkon]

Noop update

[xkon]

Changed the diff into 40966.4.diff to fix the issues that @afercia mentioned.

Hopefully this will pass. I already did my tests but more tests are always welcome.

Best regards,
Konstantinos

[swissspidy]

Few notes:

• Network admin for themes is probably affected too
• The first patches don't work because of a11y rasons
• 40966.4.diff​ is too broad. $('.updating-message') could be anything

What we really need is a way to disallow a specific theme/plugin that has already been updated.

The bulk action handler bails early if an item doesn't have the update class (like, when a plugin doesn't have an update). In theory this be used to fix this bug here. However, wp.updates.updatePlugin doesn't remove that update class after a successful update.

40966.5.diff​ fixes that by removing update with updating during the process.

[xkon]

I just tried your patch @swissspidy and I'm still able to hit the Apply button resulting in plugins update failed.

I thought the idea was to not being able to re-send the update command by 'disabling' the Apply button so we don't get update errors. That's why by reading if the 'updating-message' exists somewhere in the page which is a class added only when something is currently being updated the button is just doing nothing.

Anyway I'm new to this so I might be missing something. Though the .4 patch was cancelling the Apply action and no more update failures where showing. The .5 you uploaded is still giving Errors.

This is by hitting the Apply button 5 times in a row.

Best regards,
Konstantinos

[xkon]

@swissspidy , now that i see this again with fresh eyes - do you want me to apply your patch and rework my js on top of your new css classes ? Just in case that makes it good to get it onboard as well?

[swissspidy]

@xkon Sure, feel free to build upon 40966.5.diff​.

[bookdude13]

Refreshed patch

[bookdude13]

Hey all! I'm new here, and I hope I'm not stepping on anyone's toes. Patch refreshed. This works for me with a plugin updating. Not sure how to test the theme side. Note that I did not do the css selector change at the end of 40966.5. I wasn't sure if that was still necessary since the last post here was a couple years ago.