Make WordPress Core

Opened 4 years ago

Closed 5 weeks ago

#19159 closed enhancement (fixed)

When Inactive Widgets list gets long it is hard to clear

Reported by: westi Owned by: cdog
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.3
Component: Widgets Keywords: has-patch has-screenshots
Focuses: ui, accessibility Cc:


If you have a lot of inactive widgets it can be really hard manual labour to remove them all.

It would be much nicer if there was a delete all button like this plugin adds http://wordpress.org/extend/plugins/remove-inactive-widgets/

Attachments (8)

19159.diff (4.5 KB) - added by cdog 3 years ago.
19159.jpg (19.9 KB) - added by cdog 3 years ago.
19159.2.diff (4.8 KB) - added by cdog 3 years ago.
19159.3.diff (4.9 KB) - added by cdog 3 years ago.
19159.4.diff (4.9 KB) - added by cdog 3 months ago.
19159.5.diff (5.6 KB) - added by cdog 2 months ago.
19159.5.diff.png (44.8 KB) - added by DrewAPicture 7 weeks ago.
w/ 19159.5.diff
19159.6.diff (6.8 KB) - added by cdog 6 weeks ago.

Download all attachments as: .zip

Change History (40)

#1 @cdog
3 years ago

  • Cc catalin.dogaru@… added

#2 @cdog
3 years ago

  • Keywords has-patch added; needs-patch removed

attachment:19159.diff adds support to clear asynchronously the Inactive Widgets list. It works properly with JavaScript disabled too. The output can be previewed here: attachment:19159.jpg.

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

3 years ago

3 years ago

#3 @cdog
3 years ago

  • Owner set to cdog
  • Status changed from new to accepted

#4 @cdog
3 years ago

  • Keywords ux-feedback added

3 years ago

#5 @cdog
3 years ago

  • Keywords needs-testing added

attachment:19159.2.diff updates my previous submission so the patch can be tested against the latest revision (23132).

While updating I've noticed a possible bug with the spinner which I reported here: ticket:22839.

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

3 years ago

#6 @cdog
3 years ago

attachment:19159.3.diff uses .css() to set the display property to inline-block instead of .show() to avoid ticket:22839.

#7 @nacin
3 years ago

  • Cc lessbloat added

Adding lessbloat as CC for a potential user test.

#8 @ocean90
3 years ago

#23080 was marked as a duplicate.

#9 @chriscct7
3 months ago

  • Focuses ui added
  • Keywords dev-feedback added; westi-likes 3.4-early removed
  • Severity changed from minor to normal

#10 @DrewAPicture
3 months ago

  • Keywords needs-refresh added; needs-testing removed

Patch needs a refresh.

3 months ago

#11 @cdog
3 months ago

Here it is, attachment:19159.4.diff applies against the current revision. Going to document the code. Let me know if there's anything more to update like texts or ui elements.

Last edited 3 months ago by cdog (previous) (diff)

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

3 months ago

#13 @SergeyBiryukov
3 months ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 4.4

#14 follow-up: @alexvorn2
2 months ago

  • Keywords needs-patch added; has-patch removed

Note: Making the inactive sidebar array empty is not enough! We need also to clear the widget settings from the database of each widget ID from Inactive Widgets Sidebar.

The new patch should check every widget id from it's own database value and remove itself from the table.

2 months ago

#15 in reply to: ↑ 14 @cdog
2 months ago

  • Keywords has-patch added; needs-patch removed

alexvorn2 thanks, that was missing from the patch. Anyone wanting/able to review the patch? I'm also interested if there's any other way to remove a widget by its ID without parsing it.

#16 follow-up: @chriscct7
2 months ago

Couple things so far.

Any thing that uses $_POST should be isset checked, so for example before

unset( $_POST['removeinactivewidgets'], $_POST['action'] );

the POST value should be checked to ensure it exists

Also because this is a new patch, it should follow the current WP code guidelines, so

 if ( ! current_user_can( 'edit_theme_options' ) ) 
 	wp_die( -1 );

should be

 if ( ! current_user_can( 'edit_theme_options' ) ) { 
 	wp_die( -1 );

#17 in reply to: ↑ 16 @cdog
2 months ago

chriscct7 got it. Let's see if there's anything more that needs to be updated and I'm going to apply these changes.

#18 follow-up: @alexvorn2
2 months ago

you should prevent clicking multiple times/spamming the "remove inactive widgets" button.

#19 in reply to: ↑ 18 @cdog
2 months ago

alexvorn2 when should the button be disabled? While processing the request, when the list is empty, or both?

Also regarding the UI and texts, is there anything that should be updated?

Last edited 2 months ago by cdog (previous) (diff)

#20 follow-up: @DrewAPicture
7 weeks ago

  • Keywords has-screenshots added

Tested 19159.5.diff and it seems to work as expected. The first time I did it, I missed the spinner because I didn't realize it was up by the "Inactive Widgets" section title.

I'd maybe move the spinner down next to the button to more closely match the widget-saving experience.

#21 in reply to: ↑ 20 @cdog
7 weeks ago

DrewAPicture, great! The spinner is already there, the patch only toggles it. Should we move it along with the button?

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

7 weeks ago

#23 follow-up: @afercia
7 weeks ago

Hello. For accessibility, disabling/hiding/removing from the DOM a focused element is always problematic: where the focus will land? Browsers behave differently, Firefox will try to keep focus "in place" meaning as keyboard users will start tabbing again it will focus the closest focusable element. Chrome will completely lose focus and users will have to start tabbing again from the document root.

So, while we should prevent multiple AJAX calls, I wouldn't recommend to disable the button after it gets activated.

Instead, when the Inactive Widgets list is initially empty, maybe the button shouldn't be displayed at all. It's a bit odd to see an empty list and a button to clear the empty list, see screenshot:


Spinner: the one close to the "Inactive Widgets" title is used when dragging widgets and can't be moved. So maybe the new button should use a new spinner placed on the right.

Some additional considerations so far:

  • not an expert but what about error handling? What happens if clearing the list fails and what kind of visual feedback is given to users?
  • together with a visual feedback, I'd consider to add audible success/error messages using wp.a11y.speak
  • CSS: not sure why the button's container needs to clear previous floats, if it's really needed the CSS selector looks a bit overqualified to me and maybe could be simplified in just .remove-inactive-widgets. Same for the selector used to hide it when the wrapper is closed
  • there's a space to remove here: $( '.spinner', sb ).removeClass(' is-active' );
Last edited 7 weeks ago by afercia (previous) (diff)

#24 @afercia
7 weeks ago

  • Focuses accessibility added

#25 in reply to: ↑ 23 @cdog
6 weeks ago

afercia, constructive feedback, thanks for following up! Agreed, disabling the button can cause focus issues. Regarding the soultion you have proposed, the button becomes visible when the list in not empty. But what happens when you clear it, does it disappear? It will have the same effect as disabling the button (losing focus). In my opinion, the button should always stay visible. If it's too much for the interface, we can move the description to a confirmation dialog or even remove it entirely. What about disabling the button only when the list is empty and simply preventing multiple AJAX calls during an action? I don't think that the empty list with the button looks odd at all. See a similar example from OS X:


Everything else looks good to me. I'am not really sure about how the error handling should work, but I can start with adding a new spinner and fixing the rest of the things for now.

#26 follow-up: @afercia
6 weeks ago

I'd agree the button should stay visible and enabled while clearing the list, to avoid focus loss. About the second issue, personally I like interfaces that show controls only when you need them so I wouldn't display the button when the list is initially empty but I'll leave this to people more expert than me about UI design :)

#27 in reply to: ↑ 26 @cdog
6 weeks ago

afercia, I'm going to do it as you suggested, it can be changed later if needed. Thanks again for helping out :) I'm not exactly the person who takes decisions here, that's why I ask before starting to work on something. I'm volunteering my own time to give back to the WordPress community.

#28 @wonderboymusic
6 weeks ago

  • Keywords needs-refresh added; ux-feedback dev-feedback removed

#29 follow-up: @DrewAPicture
6 weeks ago

@cdog Can you please refresh the patch? We're edging up to 4.4 beta 1 in about a day and a half, FYI.

#30 in reply to: ↑ 29 @cdog
6 weeks ago

DrewAPicture, on it.

6 weeks ago

#31 @cdog
6 weeks ago

  • Keywords needs-refresh removed

attachment:19159.6.diff​ applies against the current revision.

What I've done:

  • Refreshed the patch.
  • Refactored the code as suggesed by chriscct7 and afercia.
  • Added a new spinner as discussed with DrewAPicture and afercia.
  • The button gets disabled when the list is empty to prevent multiple calls as pointed by alexvorn2.
  • Moved the description paragraph outside .widget-holder (hidden by .no-js otherwise).
  • Removed unused styles from the previous patch as noted by afercia (the CSS rule clearing floats).
  • Updated the patch to work with JavaScript disabled too.

What I haven't:

  • Updated error handling to provide visual/audio feedback as suggested by afercia. This should be discussed.
  • Checked if $_POST variables are set before unsetting them as pointed by chriscct7 (unset is harmless in this case).
  • Hidden the button when the list is empty as described by afercia. To be discussed, this should target the description paragraph too.
  • Prevented multiple calls while clearing the list as pointed by alexvorn2.

@DrewAPicture, please let me know if there's anything else that should be updated until tomorrow. Thanks everyone for getting involved!

#32 @azaozz
5 weeks ago

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

In 35317:

Widgets: add a button to delete all inactive widgets simultaneously for better UX.

Props cdog.
Fixes #19159.

Note: See TracTickets for help on using tickets.