Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popovers with form controls for settings should use Cancel and Save buttons #63310

Open
afercia opened this issue Jul 9, 2024 · 10 comments
Open
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 9, 2024

Description

In the new Post summary panel, some settings previously placed within collapsible panels have been moved to Popovers.
This made more evident some pre-existing accessibility issues with this pattern, especially in terms of keyboard interaction.

There are some long established conventions when it comes to keyboard interaction and UI dialogs / popovers. To me, the two most important ones in this case are. As a user:

  • I'd expect that pressing the Escape key reverts any change I made and cancels the current operation.
  • Similarly, I'd expect that activating the X close button in a dialog or popover reverts any change I made and cancels the current operation.

Instead, when usign the keyboard in most of these popovers the only way to close the popover is to press the Escape key or the X close button but that doesn't reverts or cancel. The changed values are already saved on the onChange event. This isn;t what I would expect.

I'd like to collect some feedback from more accessibility specialists cc @joedolson. In my opinion, in this kind of popovers saving should always performed after an explicit user action. This is different from an input or other controls placed 'inline' in the UI, where it may make sense to save on change. The expectation in a popover or dialog is to explicitly Cancel or Save and I'd think the only way to improve the user experience and interaction is by adding Cancel and Save buttons.

A few screenshots of this design pattern in some of the editor popovers:

  • Post link (also known as 'permalink')
  • Post Excerpt
  • Author
  • Discussion

trunk

  • Status and visibility
  • Post publish date and time

status and visibility

Step-by-step reproduction instructions

  • Use the keyboard and interact with one of these popovers.
  • Observe the only ways to close these popovers are:
    • By pressing the Escape key
    • By moving focus to the X close button and pressing it
  • Expected behavior in both cases: any change to be discarded.
  • Actual behaviors: changes are saved

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Editor /packages/editor labels Jul 9, 2024
@afercia
Copy link
Contributor Author

afercia commented Jul 9, 2024

Note: the Excerpt popover is a special case. When testing, you will notice that changes to the excerpt are saved only after tabbing away from the textarea (onblur). This is inconsistent with other popovers and is even more disorienting. When pressing Escape after a change and with the keyboard focus still in the textarea, the changes are not saved.

Noting that the Excerpt pattern is used also for editing other properties e.g. the Template description. Screenshot:

Screenshot 2024-07-12 at 10 55 51

@roygbyte
Copy link
Contributor

Would these dialogs benefit from a "revert" action?

  • Clicking revert would reset the contents of popover to its previous state (i.e.: what it contained before being revealed by the user).
  • Clicking X would persist the displayed state of the popover (so, just like it does now)

@antonyjsmith
Copy link

antonyjsmith commented Aug 9, 2024

Why on earth does Author open a pop over to then open a drop down list? This is just another bizarre set of design decisions pushed to core half-baked.

I imagine it'll be neigh on impossible to get this reverted so I'd normally say this just needs a cancel and save button. But then if you click save and then don't update the page does it save? Painted ourselves into a corner with this one.

@joedolson
Copy link
Contributor

These interactions are incredibly murky; the usability is extremely poor, and the lack of explicit information about how to either keep or discard a change is a major problem for usability and accessibility.

@jeryj If you have a chance to look at this, I'd appreciate it; I know that Andrea is off for the rest of the month.

@Bovelett
Copy link

For all issues mentioned, I just want to add my voice to this. They are right.

Moving the excerpt up in the sidebar was well intended, but improving the UI by making it accessible is a must.

@afercia
Copy link
Contributor Author

afercia commented Aug 27, 2024

Why on earth does Author open a pop over to then open a drop down list? This is just another bizarre set of design decisions pushed to core half-baked.

Yeah. The Author one is even more puzzling. It violates one more principle for select elements explained in this Trac ticket;. Besides that, I think I've never seen in more than 20 years of professional career such an UI where users have to click to open an UI that contains only one select with no 'apply' button to confirm the selection and no Save / Cancel buttons.

WordPress is an open source project where contributions from anyone are blessed and welcomed. I'm not surprised that, sometimes, some proposed design changes aren't great. However, I'm very surprised that this was merged and released.

This change should have been gone through a broader discussion. It should have been considered better and, more importantly, gone through some user testing.

I'd like to be very clear that this isn't to blame individual contributors in this project. Rather, it's the process in place that doesn't guarantee the level of quality I'd expect for WordPress. It's the process that needs to be changed.

After more than 10 years of contribution to WordPress and 7 years of life of the Gutenberg project, I'm not sure this is the kind of design I'd like to see. It's not up to what I would expect in terms of quality in WordPress. I think I'm not alone. Reporting some random comments from this X thread:

  • not intuitive
  • What happens if I click the X? Does it save my excerpt or delete it?
  • This is horrible UX that was unnecessary. Now I get to explain this to clients and stakeholders that yet another thing was messed with that worked just fine before for no reason
  • Can anyone tell me who let that hidden excerpt thingy pass into prod?
  • I appreciate some of the changes to simplify this screen, other are odd, especially put single drop downs for author and template in popovers.
  • the delete link was hidden where clients will never find it! But i like the featured image being moved up.
  • It's another crap UI change, poorly thought through and now somehow in core which will lead to a bunch of open issues and discussion wasting everyones time.
  • How on Earth did this pass design review? The X to close (or does it save?) is poor. I do wonder if there is any QC going on right now.
  • I just dealt with the excerpt section yesterday. I was baffled by the way it was displayed.
  • I’ve had to show two clients where to find this is twice already. Terrible UI
  • This seems like an accessibility issue.
  • it's absolutely a problem on a usability scale, regardless of accessibility concerns. It's just an extremely murky UX.

@jeryj
Copy link
Contributor

jeryj commented Aug 28, 2024

Preface: This is the first time I'm looking at this, so my thoughts are still forming. Here's my initial take.

I see what you mean that the interaction through the popover makes the sidebar document interactions murky. However, I think adding a Save button inside the popover may actually make it even murkier and inconsistent with other popovers.

For example, when applying a color change via a popover, the color changes are applied immediately:
Text color popover with selection in popover lining up with sidebar change

These flyout popover menus don't use save/cancel buttons, so I'm not sure that these should diverge.

For consistency, we would want a save and cancel button for all of these, but then that's going to create a lot more interactions. Also, I think a save button can make it more confusing right now, because the changes are not persisted until you save the whole post with the main Save button in the header. If we add a "Save" button, I feel like people would rightly think their change was saved when they click it within the popover, when they would actually need to save again via the header.

I agree the interactions are murky, but they are consistent at the moment. If we add the Save and Cancel buttons here, I think it will create inconsistency and confusion as well.

Some of these popovers seem a little unnecessary as well. For example, should the excerpt just be a resizeable textarea within the sidebar? Or the Edit excerpt button could focus an excerpt textarea within the sidebar?

Author popover from sidebar just a modal with a dropdown in it

Opening an Author modal with a dropdown when it could be inline dropdown seems odd to me. Maybe I'm missing something? Are there instances where this modal has more options within it?

When testing, you will notice that changes to the excerpt are saved only after tabbing away from the textarea (onblur). This is inconsistent with other popovers and is even more disorienting. When pressing Escape after a change and with the keyboard focus still in the textarea, the changes are not saved.

That's definitely extra confusing. In order to bring it in line with the others, closing this should also save whatever is within the textarea. I think a Reset button to revert the changes to whatever is saved in the database would be helpful for clarity here. I could esee

@jeryj
Copy link
Contributor

jeryj commented Aug 28, 2024

Digging into the work that implemented this in #60894. There are comments about adding Done/Cancel buttons, which I like more than Save because of the potential confusion around when it's really saved. Here's the issue about the thought processes behind these changes.

@afercia
Copy link
Contributor Author

afercia commented Aug 30, 2024

These flyout popover menus don't use save/cancel buttons, so I'm not sure that these should diverge.

I'd apply the buttons to all such popovers. Whether it's a save/cancel or apply/cancel, to me buttons are the only way to avoid any confusion and clearly provide ways to close/cancel and close/apply.

The 'React way' to set values onChange and update on the fly may be OK for inputs and controls that are part of a form that lives in an UI that is always visible. Instead, it's extremely confusing for UIs that live in a separate 'window-like' UI like a popover. A popover resembles a dialog window where I'd expect cleaer ways to:

  • Cancel the current operation and close.
  • Explicitly save and close.

Instead, the 'on the fly saving and updating' pattern is extremely confusing when used for a dialog-like UI.

@joedolson
Copy link
Contributor

I think the text 'Save' should only be used if there will actually be a change made to the database; otherwise, 'Done' or 'Apply' should be used (preferably only one of those, and consistently throughout.) Using the word 'Save' conveys a very specific sense in the UI, and I think isn't the right term in most of these cases.

I agree with @jeryj that some of these popovers seem really unnecessary, as well, but that's kind of a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Editor /packages/editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants