WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 6 weeks ago

Last modified 3 weeks ago

#41610 closed enhancement (fixed)

Widgets: Change "close" to "done?"

Reported by: melchoyce Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

While testing the image widget, a couple of the people I was testing with were worried that their widgets edits weren't "saved" or "confirmed" until the widget was closed. One person called this out explicitly, and another acted like closing “saved” the widget.

To assuage fears of your new widget being unsaved, maybe “close” should be say like “done” instead? If this is a trend, relabeling could help sooth things over for worried people without making any sort of technical change.

Change History (27)

#1 @westonruter
2 months ago

To confirm, this is on the widgets admin screen, right?

@melchoyce Yeah, you can close a widget without having saved it. In fact, there is no AYS dialog when leaving the widgets admin screen when you have unsaved changes. It would be nice if the “Save” button were changed to “Saved” while being disabled initially, and then when a change is made to a widget to cause the button to enable and show “Save”. Upon clicking “Save” then turn the button to be disabled and “Saved” again once the save is complete. If there is an error, then some indication should be shown there as well. I think “Saved” would be more reassuring than “Close” or “Done”.

This to me sounds like it would be a duplicate of #23120.

By the way, I think the “close” link (which appears next to “Delete”) is entirely redundant since it has the same operation as clicking on the title bar above.

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

#2 @melchoyce
2 months ago

To confirm, this is on the widgets admin screen, right?

Nope, this is about widgets in the Customizer. Sorry for the confusion.

This to me sounds like it would be a duplicate of #23120.

I don't think so, because we wouldn't actually be indicating that saving is happening — it's just a label change to make folks feel like they're done editing their widgets for the time, since the Customizer doesn't actually have a "save." It's close, but not totally the same thing.

By the way, I think the “close” link (which appears next to “Delete”) is entirely redundant since it has the same operation as clicking on the title bar above.

It is, but I don't think that's a problem. However, we could try testing removing it entirely and see how folks handle that.

#3 follow-up: @westonruter
2 months ago

While testing the image widget, a couple of the people I was testing with were worried that their widgets edits weren't "saved" or "confirmed" until the widget was closed. One person called this out explicitly, and another acted like closing “saved” the widget.

Did the users locate the widget in the preview before making a change? If they don't see the change they made in the preview, then certainly I can understand why they would be concerned about the change not being saved. Maybe #39389 could then help there in that case by scrolling the widget into view?

#4 @melchoyce
2 months ago

Let me go back and rewatch 👍

#5 in reply to: ↑ 3 @melchoyce
2 months ago

Replying to westonruter:

While testing the image widget, a couple of the people I was testing with were worried that their widgets edits weren't "saved" or "confirmed" until the widget was closed. One person called this out explicitly, and another acted like closing “saved” the widget.

Did the users locate the widget in the preview before making a change? If they don't see the change they made in the preview, then certainly I can understand why they would be concerned about the change not being saved. Maybe #39389 could then help there in that case by scrolling the widget into view?

Wanted to follow up on this:

One person kept closing every widget when she was done looking at them. After adding the image widget, she closed it using the "close" link and then immediately went and scrolled in the preview to look at it.

The second person also did not look at the preview when he closed the widget in the sidebar. Additionally, he said:

The close button... I dunno, maybe change it... I'm not sure. Initially when I'm finished completing the widget, hitting "close," it doesn't strike me as instantly as it being saved. When I'm finished interacting with the widget, I don't know if it's going to be saved. So maybe instead of being "close" maybe it's "done." When I hit close, maybe if there's some way to have a confirmation that everything I've done is being saved.

So, I think #39389 might help here.

#6 @timmydcrawford
7 weeks ago

Here is a branch to test out "Close" vs "Done"

https://github.com/xwp/wordpress-develop/pull/250

#8 @westonruter
7 weeks ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.9

@timmydcrawford what if someone clicks “Done” without having saved the widget? Maybe someone would click “Done” without having clicked “Save” first? Maybe it should only say “Done” only in the Customizer?

I've amended the PR to prototype the addition of an AYS dialog on the widgets admin screen if you try to leave without having saved all the widgets you modified. Additionally, a widget's Save button is disabled when the widget is first expanded, but then once a field is modified then the button is enabled, until hitting Save causes the dirty flag to be cleared and the button re-disables: 715884d.

This would fix something that bugged me when testing media widgets where I'd select an image for the Image widget, but then I'd forget to hit Save on the widgets admin screen so the image never got saved to the DB.

This would essentially also fix #23120, and it seems like something that would be necessary if we add “Done” here. Maybe the “Done” link would only be shown when the widget is saved on the widgets admin screen? In the Customizer the “Done” link could always be displayed because whenever you make a change it is written into the changeset. I've implemented this change in 9a4a607.

It feels much better now interacting with widgets on the widgets admin screen with these changes.

#9 @melchoyce
6 weeks ago

Maybe it should only say “Done” only in the Customizer?

+1

Maybe the “Done” link would only be shown when the widget is saved on the widgets admin screen?

Would the label dynamically update?

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


6 weeks ago

#12 @westonruter
6 weeks ago

The “Done” link would persistently show regardless of the widget's dirty state, as likewise the “Save” button is not shown since changes are autosaved.

#13 @melchoyce
6 weeks ago

Okay, let's go with that. 👍 Thanks @westonruter.

#14 follow-up: @westonruter
6 weeks ago

@melchoyce should the button say “Saved” when disabled?

#15 in reply to: ↑ 14 @melchoyce
6 weeks ago

Replying to westonruter:

@melchoyce should the button say “Saved” when disabled?

Yeah, that makes sense to me.

#16 @westonruter
6 weeks ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 41352:

Widgets: Add dirty state tracking for widgets on admin screen.

  • Mark a widget as dirty when a field input triggers a change or input event; clear dirty state when widget is successfully saved.
  • Disable Save button and re-label "Saved" when widget not dirty.
  • Show AYS dialog when leaving widgets admin screen with unsaved changes.
  • When widgets are dirty, expand all unsaved widgets at AYS check and focus on first one.
  • Change "Close" link to "Done"; hide link when widget is dirty and reveal when saved.
  • The "Done" link persistently appears in the Customizer even after making a change (when the widget is dirty) because changes are autosaved into the changeset.
  • Prevent saving widget when form fails checkValidity.
  • Fix frequency of triggering of change event on the rich Text widget's textarea limited now to when there are actual changes.
  • Add a class of widget-dirty to widget containers when the widget has unsaved changes.

Props westonruter, timmydcrawford, melchoyce.
Fixes #41610, #23120.

#17 @Otto42
6 weeks ago

@westonruter Think you broke something pretty bad here.

Calls to wp-admin/load-scripts.php are now giving me an error:

Fatal error: Uncaught Error: Call to undefined function wp_json_encode() in .../wp-includes/script-loader.php on line 675

wp_json_encode is in the wp-includes/functions.php file, which the load-scripts.php entry point does not load.

The result of this is that a lot of the wp-admin is not working because the script concatenater code no longer returns any scripts. Breaks, oh, pretty much everything in the wp-admin that uses javascript libraries.

Noticed this on WordPress.org in the plugins admin area first.

Last edited 6 weeks ago by Otto42 (previous) (diff)

#18 @westonruter
6 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@Otto42 thanks for that. I'll fix.

#19 @westonruter
6 weeks ago

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

In 41360:

Widgets: Prevent fatal error due to calling undefined wp_json_encode() when requesting the load-scripts.php endpoint.

Props Otto42.
Amends [41352].
Fixes #41610.

#20 @westonruter
6 weeks ago

Now I understand what why did_action( 'init' ) appears all over the place in script-loader.php!

#21 @Otto42
6 weeks ago

Fix confirmed when deployed to w.org. Thanks!

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


6 weeks ago

#23 follow-up: @afercia
4 weeks ago

I'm not sure I fully agree with the solution implemented here. While I agree there should be an indication of the saved state, I think the button isn't the proper place for that.

I've recently noticed this new design trend also in other places across the admin, and I'd recommend to reconsider it. As I see it, the main issue here is that buttons (and other UI controls) that perform an action should be exclusively labeled to indicate the available action.

Instead, with this change (and also in other places in the admin), the button label is actually a mix of the available action and the UI state:

https://cldup.com/Qe-yAeIXT7.png

Mixing these two different information in the same place defeats the purpose of properly labeling UI controls and is potentially confusing for users, not to mention it's far from ideal for accessibility.

In other places in the admin things are even more confusing, when the UI control label is actually the value of the underlying option. Thinking, for example, at the Publish option in Gutenberg which says "Immediately" or uses the publish date. UI controls labels should make sense even when read out of context: "Immediately" or "Saved" don't tell me anything about what the available action is.

UI controls that perform an action should be used just to perform actions. Not to indicate the underlying state or value. A state indicator is a completely different thing from a button (or other UI controls) and should be separated from the control.

I know this is not completely new in WP, I'm commenting here just because it's the most recent implementation of this new trend I'm aware of. I'd like to see a broader discussion on this issue involving both the design and accessibility team.

As a general consideration, there's no need to reinvent the wheel trying to do fancy things. Design should explore new solutions, but always respecting the specifications. A button is a button, just that. Please respect the elements semantics and use HTML elements for what they're intended for.

Apart from these considerations, disabling the button causes a focus loss, one of the worst experiences for keyboard and screen reader users. This was mentioned several times: for accessibility, disabling, hiding, or removing from the DOM an UI control that currently has focus should be avoided. This should be tested for accessibility to evaluate the focus loss impact, since browsers are making some progress in this regard and they try to keep focus in place when possible. I'll propose some testing during the next accessibility meeting.

Aside: not sure why both the input and change events are used; input should be enough, unless I'm missing something.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 weeks ago

#26 in reply to: ↑ 23 @westonruter
3 weeks ago

Replying to afercia:

Aside: not sure why both the input and change events are used; input should be enough, unless I'm missing something.

Both events are used because changing radio buttons does not trigger input, only change (see MDN). Also, custom JS components will often sync changes into hidden inputs and trigger change events on them, without any input.

#27 @afercia
3 weeks ago

Ah it's about radio buttons, ok!

Note: See TracTickets for help on using tickets.