Skip to content
New issue

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

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

Already on GitHub? # to your account

Custom field check box automatically changes the context on user focus #15320

Closed
karlgroves opened this issue Apr 30, 2019 · 15 comments · Fixed by #15688
Closed

Custom field check box automatically changes the context on user focus #15320

karlgroves opened this issue Apr 30, 2019 · 15 comments · Fixed by #15688
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Bug An existing feature does not function as intended

Comments

@karlgroves
Copy link

Custom field check box automatically changes the context on user focus

  • Severity: High
  • Affected Populations:
    • Blind
    • Low-Vision
    • Motor Impaired
    • Cognitively Impaired
  • Platform(s):
    • All / Universal
  • Components affected:
    • Tools and Options

Issue description

The "Custom Fields" checkbox within the "Options" dialog causes an
unexpected page refresh when the user selects it, and consequently
resets the focus position to the top of the page.

This is not expected behaviour for checkboxes, and the sudden change of
context may be confusing for assistive technology users, while the loss
of focus position will force keyboard users to manually navigate back to
where they were before.

Remediation Guidance

If it's not possible to update the page in the background, as happens
with other checkboxes in the "Options" dialog, then the user should be
warned (with visible text, not visually-hidden) that selecting this
option will close the options dialog and reload the page.

Alternatively, an "Apply Changes" button could be added to the dialog,
which confirms the user's changes for all checkboxes (i.e. none of the
checkboxes cause background updates or page reloads until the user
explicitly presses the Apply button).

Relevant standards

Note: This issue may be a duplicate with other existing accessibility-related bugs in this project. This issue comes from the Gutenberg accessibility audit, performed by Tenon and funded by WP Campus. This issue is GUT-65 in Tenon's report

@gziolo gziolo added the Needs Accessibility Feedback Need input from accessibility label Apr 30, 2019
@melchoyce
Copy link
Contributor

Screenshot from the full report:

image

@afercia
Copy link
Contributor

afercia commented May 5, 2019

+1
Note: when there are unsaved changes to the content, a JavaScript confirm dialog will ask if you want to reload the page but that's unrelated. To reproduce, save your changes first and then click the
"Custom Fields" checkbox.

@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). User Experience (UX) and removed Needs Accessibility Feedback Need input from accessibility labels May 5, 2019
@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label May 6, 2019
@sarahmonster sarahmonster added Needs Dev Note Requires a developer note for a major WordPress release cycle Needs Technical Feedback Needs testing from a developer perspective. and removed Needs Design Feedback Needs general design feedback. labels May 7, 2019
@sarahmonster
Copy link
Member

The design team discussed this during a triage session in Slack today (Note: You may need a Slack account to log in.)

The best solution, from both an accessibility and a user experience perspective, would be to avoid this page refresh altogether, since it's not in line with user expectations of how checkboxes typically work on the web. (Checking a checkbox doesn't usually refresh the page.)

If the page refresh is strictly unavoidable, we could add some warning text to the label: "(Requires page refresh)” or similar, but this is something of a band-aid solution. First, we'd like to explore ways of avoiding that refresh altogether.

Technical feedback and/or explorations here would be super helpful!

@karmatosed karmatosed added Needs Design Feedback Needs general design feedback. and removed User Experience (UX) labels May 7, 2019
@karmatosed
Copy link
Member

I am removing the 'User Experience (UX)' label as part of a label cleanup. It's not being used anymore consistently so let's try and keep to 'needs design' and 'needs design feedback'. If we find a need for another label we can consider it but having those 2 should cover this.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label May 7, 2019
@noisysocks
Copy link
Member

The checkbox works by registering and de-registering the pre-existing postcustom meta box (#11084). When this meta box is registered, the WordPress backend will include the necessary HTML, JS, and CSS for the Custom Fields form. When any meta box is registered, WordPress will instruct the block editor to watch for post saves and, upon save, make an additional POST request which updates the post's meta boxes.

Custom Fields are disabled by default. We want to avoid including unnecessary frontend resources and making two requests per save in the majority case. Therefore, we decided (#10676 (comment)) to require a page reload to toggle the setting.

We could, in theory, avoid the page refresh by manually injecting the HTML, <script> and <link> tags into the page when the option is toggled on. It would be challenging to maintain such an approach, though.

We could also look into rebuilding the Custom Fields UI to be a React-based component that uses the REST API. This would require some work, though, as we would need to add support for Custom Fields to the REST API.

I don't think either of those two approaches are worth the development effort involved. Custom Fields is a feature without that exists only in the block editor for backwards compatibility with the classic editor.

Instead, I think it's best that we polish the existing mechanism to make it less jarring and more accessible. I'd encourage us to explore adding a Page reload is required button that appears after the checkbox is toggled.

@marekhrabe
Copy link
Contributor

Looking at the code, what about using already established DeferredOption? It visually looks like all other checkboxes but its handler (the function doing the actual work to save the new value) is called only once the modal is dismissed.

DeferredOption is currently used for the option of "Show Tips" where they only get enabled after you close the modal to prevent UI collisions.

What I imagine a flow might look like:

  • open Options
  • toggle "Custom Fields" checkbox
  • its label can be updated to mention the page reload but nothing else happens
  • close modal
  • only now we finally perform the reload

If user changes their mind and flips the option back to the initial state before they close the modal, no reload will be performed once they do close it.

This should be fairly easy to implement and leverages code already in the repository.

@marekhrabe
Copy link
Contributor

I've made #15688 to explore this variant. Let me know how it works for y'all.

@afercia
Copy link
Contributor

afercia commented May 17, 2019

@marekhrabe thanks for exploring a solution for this issue.

close modal, only now we finally perform the reload

I'm not sure this actually solves the root issue, it just changes the moment when an unexpected page reload happens 🙂

When closing any modal, the expected interaction is that focus is moved back to the button that triggered the modal. Note: we're already doing an exception here because, at that point, the More menu is closed so focus is returned to the only logical place: the More button. Triggering a page reload when would be completely unexpected, even with the info text added to the checkbox label.

I'd suggest to explore the additional button suggested in the report.

@marekhrabe
Copy link
Contributor

Agreed - I have the same feeling from the interaction but it was the easiest option in code so I thought I'll start with it. I will have a look at the button option and try something out

@gziolo gziolo removed Needs Technical Feedback Needs testing from a developer perspective. Needs Dev Note Requires a developer note for a major WordPress release cycle labels May 24, 2019
@gziolo
Copy link
Member

gziolo commented May 24, 2019

I think @noisysocks explained in-depth (#15320 (comment)) why it works the way it works. It's suboptimal and would be very hard or time-consuming to fix for something which is going to be used by a very small group of users. It's definitely not our priority.

@gziolo gziolo added the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label May 24, 2019
@marekhrabe
Copy link
Contributor

I've updated my exploration PR to use an additional confirmation button. Can somebody take a look?

@mapk
Copy link
Contributor

mapk commented May 29, 2019

Here's what the confirmation button looks like in a screencast.

notices

This is a good route. Let's restyle the notification to look more like a notification and I think we'll be good.

Maybe something like this? Notice the alignment adjustments too...

Screen Shot 2019-05-28 at 6 58 44 PM

@marekhrabe
Copy link
Contributor

Before we proceed, could @afercia confirm we are solving the issue the right way? To me it feels better both visually and in Voiceover but I’m not a regular Voiceover user so my point of view is not that important.

@mapk mapk added the Needs Accessibility Feedback Need input from accessibility label May 29, 2019
@afercia
Copy link
Contributor

afercia commented May 29, 2019

A notice (also a spoken notice) and the reload happening on user action sound good to me, thanks!

@afercia afercia removed the Needs Accessibility Feedback Need input from accessibility label May 29, 2019
@mapk
Copy link
Contributor

mapk commented May 29, 2019

Just to note here, I'm thinking a yellow notification makes more sense here. It's a warning notification rather than an information one.

Screen Shot 2019-05-29 at 10 07 20 AM

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants