WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#37563 closed defect (bug) (fixed)

On page updates: Bulk actions FTP dismissal issues

Reported by: jorbin Owned by: jorbin
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Plugins Keywords: has-screenshots has-patch needs-testing commit
Focuses: Cc:

Description

If you are using the FTP transport, do bulk updates on plugins (and likely themes. untested at this point), and dismiss the modal to enter your FTP credentials, there are some UI/UX issues.

  1. The "There is a new version of…” notices become blank
  2. The updates boxes you had checked become unchecked.

(potentially more, additional testing is needed).

Attachments (14)

37563.diff (1.8 KB) - added by afercia 3 years ago.
37563.2.diff (2.1 KB) - added by obenland 3 years ago.
37563.3.diff (2.1 KB) - added by obenland 3 years ago.
modal-with-error.gif (184.5 KB) - added by ocean90 3 years ago.
37563.4.diff (2.3 KB) - added by obenland 3 years ago.
cancel-updating-remains.gif (534.1 KB) - added by ocean90 3 years ago.
37563.5.diff (4.9 KB) - added by swissspidy 3 years ago.
37563.6.diff (2.5 KB) - added by obenland 3 years ago.
37563.7.diff (5.1 KB) - added by obenland 3 years ago.
37563.8.diff (5.1 KB) - added by swissspidy 3 years ago.
37563.9.diff (5.1 KB) - added by ocean90 3 years ago.
37563.10.diff (5.6 KB) - added by ocean90 3 years ago.
Corrected labels
wrong-label.gif (1.1 MB) - added by ocean90 3 years ago.
37563.11.diff (7.3 KB) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (44)

This ticket was mentioned in Slack in #feature-shinyupdates by jorbin. View the logs.


3 years ago

#2 @afercia
3 years ago

Looking a bit at this, I think 2) is intentional. About 1) noticed that when clicking on the "update now" link on the plugin row and the FTP credentials modal opens, wp.updates.updatePlugin runs and set the originaltext data. Easy to check because the update available notice changes and the icon starts spinning:

https://cldup.com/lFCH3KypIL.png

Instead, when bulk updating wp.updates.updatePlugin does not run, not sure if this is intentional. See below: no updating notice and no spinning icon:

https://cldup.com/oUx41URHgA.png

so, originaltext data is undefined when canceling the modal. At this point if $message.find( 'p' ).data( 'originaltext' ) is undefined maybe it should directly get the $message.find( 'p' ).html().

#3 @afercia
3 years ago

There are other cases where the originaltext thing restores the element text but then the aria-label attribute is not restored to its original value, e.g.: canceling the FTP credentials from plugin-install.php:

https://cldup.com/4xDs4dquJq.png

@afercia
3 years ago

#4 @afercia
3 years ago

  • Keywords has-screenshots has-patch needs-testing added
  • Version set to trunk

37563.diff is a possible approach to cover the cases illustrated in the screenshots above:

  • stores in the element's jQuery .data() also the original aria-label attribute for later re-use onplugin-install and plugin-install-network
  • adds a check for originaltext that can be undefined, for example when canceling the FTP credentials during a bulk update, and in this case directly gets the HTML instead of trying to get the HTML stored in .data( 'originaltext' )

Definitely needs testing and review, not sure all the edge cases are covered.

@obenland
3 years ago

#5 @obenland
3 years ago

37563.2.diff doesn't trigger the modal close action for all items in the queue anymore, but only for the one that is currently processed. It also lets individual jobs handle asking for the FTP modal. Additionally it keeps checkboxes checked until the action on the item has been completed.

@obenland
3 years ago

#6 @obenland
3 years ago

37563.3.diff uses the (notoriously unreliable) plugin slug rather than its basename because since [38168] that is not always available anymore.

#7 follow-up: @afercia
3 years ago

Seems a better approach, doesn't address the aria-label issue though. On plugin-install and plugin-install-network it needs to be restored to its initial value when canceling the FTP modal, see comment 3

#8 @ocean90
3 years ago

37563.3.diff: Removing wp.updates.maybeRequestFilesystemCredentials( event ); seems to be a bad idea because the user gets the modal with an error now, see modal-with-error.gif.

#9 @ocean90
3 years ago

  • Owner set to obenland
  • Status changed from new to reviewing

@obenland
3 years ago

#10 in reply to: ↑ 7 @obenland
3 years ago

Replying to afercia:

doesn't address the aria-label issue though

Yeah, I think your approach makes sense there. There are four more instances though where originaltext gets set. Maybe we should abstract that into a function and cll it from within those action handlers.

#11 @ocean90
3 years ago

37563.4.diff: When I now cancel the dialog the status remains at "Updating…" but only after a failed connection, see cancel-updating-remains.gif.

@swissspidy
3 years ago

#12 @swissspidy
3 years ago

37563.5.diff is an iteration on 37563.4.diff that fixes cancel-updating-remains.gif by adding back the foreach loop for 'credential-modal-cancel'. That way the message gets reset correctly.

It also fixes the aria-label on plugin-install.php so they get reset correctly after canceling the modal.

I haven't looked at 37563.diff much, but if it's a serious problem it should be easy to turn this into a new function and use that where needed.

@obenland
3 years ago

#13 @obenland
3 years ago

Fixing 11 only required adding the rejected job back to the beginning of the jobs queue rather than appending it.

#14 @swissspidy
3 years ago

@obenland Ah, I see. Leads to the same result. Wanna combine that with the aria-label changes in 37563.5.diff?

#15 @obenland
3 years ago

Hm, I'm not sure. I think I like your patch better than mine. Let me test it a bit more

@obenland
3 years ago

#16 @obenland
3 years ago

37563.7.diff is @swissspidy's patch but adding the rejected job back to the beginning of the jobs queue rather than appending it, which seems more appropriate to me. We probably don't need to loop over all jobs and trigger the modal-cancel action, but safe is safe.

@swissspidy
3 years ago

#17 @swissspidy
3 years ago

37563.8.diff is 37563.7.diff but with my copy paste error fixed in tests/qunit/fixtures/updates.js.

@ocean90
3 years ago

#18 follow-up: @ocean90
3 years ago

  • Owner changed from obenland to afercia

37563.9.diff is 37563.8.diff but removes the "now" suffix. The patch works as expected for me.

@afercia, @jorbin: Does it fix the issue(s) for you too?

#19 @ocean90
3 years ago

  • Keywords commit added

#20 in reply to: ↑ 18 @afercia
3 years ago

Replying to ocean90:

but removes the "now" suffix

The most important thing is having the aria-label correctly synced with the button text with the addition of the plugin name to give context. Not restoring "now" is really not an issue :)

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


3 years ago

@ocean90
3 years ago

Corrected labels

@ocean90
3 years ago

#22 @ocean90
3 years ago

  • Keywords commit removed

37563.10.diff corrects the labels for "now" actions. But there is an issue in the list table, see wrong-label.gif. An aria-label gets added to the notice.

Last edited 3 years ago by ocean90 (previous) (diff)

@ocean90
3 years ago

#24 @ocean90
3 years ago

  • Keywords commit added

Forgot to mention that 37563.11.diff fixes also the wrong aria-label for plugin network activation buttons.

#25 @ocean90
3 years ago

  • Owner afercia deleted

This ticket was mentioned in Slack in #feature-shinyupdates by swissspidy. View the logs.


3 years ago

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


3 years ago

#28 @ocean90
3 years ago

  • Owner set to jorbin

#29 @jorbin
3 years ago

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

In 38221:

Updates: Improve experience for Bulk Actions when FTP is dismissed.

Before this change, when a bulk update was canceled due dismissing the FTP credentials modal, part of the actions didn't get canceled. This meant the "There is a new version of…” notices become blank and the updates you had checked became unchecked. Now, the notices remain and you are essentially returned to the screen you had before. Strings are also updated to improve ARIA usage.

Fixes #37563.
Props ocean90, swissspidy, obenland, afercia.

#30 @jorbin
3 years ago

In 38222:

Updates: Improve experience for Bulk Actions when FTP is dismissed.

Merges [38221] to the 4.6 branch.

Before this change, when a bulk update was canceled due dismissing the FTP credentials modal, part of the actions didn't get canceled. This meant the "There is a new version of…” notices become blank and the updates you had checked became unchecked. Now, the notices remain and you are essentially returned to the screen you had before. Strings are also updated to improve ARIA usage.

Fixes #37563.
Props ocean90, swissspidy, obenland, afercia.

Note: See TracTickets for help on using tickets.