WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 9 days ago

#40735 closed defect (bug) (fixed)

Events widget form should close after successful search

Reported by: iandunn Owned by: azaozz
Milestone: 4.8 Priority: normal
Severity: normal Version: trunk
Component: Administration Keywords: has-patch
Focuses: ui, accessibility Cc:

Description

The Events widget on the Dashboard screen has a form where users can search for a location. Originally (in the feature plugin), the form was automatically hidden with JavaScript after a successful search was performed.

We eventually changed that so that it remains open, in order to avoid hurting accessibility when the focus gets lost. That's where things stand today. More background is available from the original GitHub issue.

Leaving the form open is good for a11y, but bad for UX. Many users expect the form to automatically close after they get a successful result, because it is no longer needed, and takes up space.

Is there a way that we can improve the UX without hurting a11y?

cc @melchoyce, @mapk, @afercia

Attachments (7)

40735-button-link.diff (608 bytes) - added by iandunn 2 weeks ago.
Removes unneeded class to avoid unwanted background on click
40735-form-ux.diff (1.7 KB) - added by coreymckrill 10 days ago.
40735.diff (7.9 KB) - added by afercia 10 days ago.
40735.css.diff (2.3 KB) - added by afercia 10 days ago.
40735.2.diff (8.2 KB) - added by coreymckrill 10 days ago.
40735-spinner.diff (353 bytes) - added by iandunn 9 days ago.
Restore default Core spinner margin-top, remove padding-bottom
40735.3.diff (741 bytes) - added by afercia 9 days ago.
Adjusts spinner position.

Download all attachments as: .zip

Change History (33)

#1 @afercia
2 weeks ago

Hm I'd say also from an usability point of view maybe it shouldn't close. What if a user changes idea and wants to search for a different location after a successful search? He/she would be forced to re-open the form again and again...

#2 @iandunn
2 weeks ago

What if a user changes idea and wants to search for a different location after a successful search? He/she would be forced to re-open the form again and again...

Is there an a11y issue there that I'm missing? It sounds like a UX issue to me. I think it's still a valid point, but I want to make sure I understand the reason.

I think the tradeoffs that impact the decision would be different if it's UX vs UX, rather than UX vs a11y.

#3 @iandunn
2 weeks ago

Er, I misread your comment. You said "usability", not "accessibility". Ok, so with that in mind, I think the factors in the decision look like this:

Pros Cons
UX: some users expect it to close a11y: Focus is lost
UX: user doesn't have to manually close form UX: User has to manually re-open to search again
UX: user isn't distracted by unnecessary element / clutter

The table is just to help clarify exactly what the pros and cons are. It doesn't mean that all of those items should weighted equally, though. In my mind the a11y issue should be weighted heavily.

I'll edit this comment to update the list if there are any changes suggested in other comments.

#4 follow-up: @afercia
2 weeks ago

To clarify from an a11y perspective: closing the form after a successful search would be fine if only there was a suitable place to move focus back to. If we move focus back to the toggle button, then screen readers wouldn't announce the speak message "City updated..." because they would start announcing again the toggle button...
I wouldn't know where else we could move focus back.

Unrelated: just noticed when clicking the "Cancel" button-link, it takes a background #eee inherited from button:active. I think that's because the button should use only the class button-link and not button button-link.

#5 @obenland
2 weeks ago

Could we make it dependent on whether the search returned results? I expected the for would close, personally, but could see users wanting to check other cities if a search returned no results.

#6 @afercia
2 weeks ago

That could help (was thinking it already worked that way?) and maybe we're also overthinking a bit :) After all, this is something users will "set once and forget" and all the following times they visit the Dashboard, the form will be closed.

@iandunn
2 weeks ago

Removes unneeded class to avoid unwanted background on click

#7 in reply to: ↑ 4 @iandunn
2 weeks ago

Replying to afercia:

Unrelated: just noticed when clicking the "Cancel" button-link, it takes a background #eee inherited from button:active. I think that's because the button should use only the class button-link and not button button-link.

You're right, it was the extra button class. Fixed in 40735-button-link.diff.

#8 @iandunn
2 weeks ago

Currently the form is only closed when the page is first loaded and a location is saved. If the user opens it manually, it's never closed until they manually close it or refresh.

I really like the idea of closing it after successful searches, but leave it open after unsuccessful ones.

That doesn't solve the focus problem when there is a successful search, though.

Last edited 2 weeks ago by iandunn (previous) (diff)

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


13 days ago

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


10 days ago

#11 @afercia
10 days ago

So, as per the Slack conversation linked above, if the form really needs to be closed after a successful search (I have my doubts this would be a better UX) then the only place where moving focus back makes sense is the toggle button. To give a chance screen readers to announce the wp.a11y.speak() message after the successful search instead of starting to announce the toggle button, the only thing we can do is add the parameter assertive to the wp.a11y.speak() call.
Not 100% sure this would fully work from an accessibility perspective, it would need some testing to make sure it doesn't introduce an accessibility regression.

#12 @coreymckrill
10 days ago

40735-form-ux.diff hides the form whenever the widget is rendered and has a valid location. If the render was user-initiated (as opposed to when the screen first loads), the toggle button gets focus after the render is complete and the form is hidden. This upgrades the wp.a11y.speak call for city_updated to assertive so that it won't get interrupted by the focus change. It also fixes a bug where the location was announced as "object" instead of the city name.

Question: Since the form will always be hidden after a successful search, should we do anything with the form focus/content if it is re-opened? Right now the focus stays on the toggle button, and the form input still contains the previous search value.

cc: @iandunn @afercia

#13 @iandunn
10 days ago

I tested the patch and it worked well for me.

@afercia, do you think 40735-button-link.diff and 40735-form-ux.diff are ready? I'd love to get them in to beta2 so they can be tested.

#14 @afercia
10 days ago

@iandunn @coreymckrill tested with Safari + VoiceOver and seems good to me: the speak message gets announced even if focus is moved back on the toggle button, see below:

https://cldup.com/Ay7ybAJlL4.png

That's what aria-live="assertive" is supposed to do: interrupt anything currently announced by the screen reader, so I'm pretty confident it works correctly also with other screen readers.

Noticed a couple minor things:

  • the cancel button is a bit misaligned
  • there are several aria-hidden="false", not sure why they're used. Will check better later, but maybe they're not strictly necessary?

#15 @afercia
10 days ago

there are several aria-hidden="false", not sure why they're used

Replying to myself :) OK I understand now, and using aria-hidden also as CSS target to toggle visibility is a good pattern! Just wondering if the universal selector * could be avoided, see below:

.community-events-errors[aria-hidden="true"],
.community-events-errors *[aria-hidden="true"],
.community-events-loading[aria-hidden="true"],
.community-events[aria-hidden="true"],
.community-events *[aria-hidden="true"] {
	display: none;
}

Since CSS is read and used by browsers right-to-left that translates in a somewhat expensive operation.

#16 @afercia
10 days ago

Tested a bit more, there is one thing still to address for better accessibility. Focus needs to be moved back to the toggle button even when pressing the "Cancel" button and the form closes.

I've made a few seconds video to clarify what happens, and I'd recommend everyone to have a look at it to better understand why, as accessibility people, we always insist on focus management :)

After pressing "Cancel" and using Control-Option-Right arrow as screen reader users normally do to navigate content instead of using Tab, there's a very evident focus loss: all the events list get skipped. See:
https://cldup.com/-_73zKWMs5.mp4

This can be fixed very easily, the most important things is everyone starts considering this kind of issues, especially now that WordPress is starting to move towards JavaScript-based user interfaces.

P.S. to reproduce, just use Control-Option-Space to press the "Cancel" button and then Control-Option-Right Arrow to move to the next content. Documentation on VoiceOver commands and gestures: https://www.apple.com/voiceover/info/guide/_1131.html

Last edited 10 days ago by afercia (previous) (diff)

@afercia
10 days ago

#17 @afercia
10 days ago

  • Keywords has-patch has-screenshots added
  • Milestone changed from Awaiting Review to 4.8
  • Type changed from enhancement to defect (bug)

I went ahead: 40735.diff unifies the previous 40735-button-link.diff and 40735-form-ux.diff and also does the following:

  • moves focus back to the toggle button when pressing "Cancel", there are several ways to do this please do feel free to change it and improve it if you prefer a different pattern
  • fixes a few mixed spaces and tabs
  • HTML attribute values are string not boolean, see aria-hidden, aria-expanded (minor because browsers take care of this so it's just for the sake of being a bit more cleaner)
  • a few JS standards
  • fixes one typo (Javascript to JavaScript) and a few minor things in the docblocks and inline comments

Also to consider:

  • isVisible was confusing for me at first, had to read some underscorejs docs to have clear that for JavaScript object the iteratee function arguments are: value, key, list; I'd consider to rename it, maybe visibility or something less confusing; after all it's just the value set in the elementVisibility object whose keys/values are element and visibility
  • maybe the two var at the top should be unified in just one
  • minor optimization: instead of getting multiple times some elements, maybe I'd try to get them just once, see for example how many times .community-events-toggle-location is targeted with jQuery

40735.css.diff
is optional :) and tries to improve the form label, input field, "Submit", and "Cancel" button alignments. To consider:

  • should the "Cancel" button link be underlined or not? Currently, there's a CSS conflict with another rule; also, after the removal of the .button class, padding is now different
  • please consider to avoid introducing new colors: the event-none border color was #0070AE which is not used anywhere in core and I've changed it to #00a0d2 which is the color already used for notice-info

Changing ticket type to "bug" as the UX issue should be addressed for 4.8, as per yesterday's discussion on Slack.

@afercia
10 days ago

#18 @afercia
10 days ago

In IE 11 the alignment of the toggle button and a few other alignments are a bit off (see screenshot below). I've updated 40735.css.diff to try to fix it.

https://cldup.com/WCPihUFtuA.png

#19 @iandunn
10 days ago

Thanks @afercia! I don't think I'll have time to thoroughly test those before beta2, but I read through your comments and the patches and it all sounds good to me.

@coreymckrill
10 days ago

#20 @coreymckrill
10 days ago

40735.diff looks good, and works for me. I added 40735.2.diff to address these two items:

  • maybe the two var at the top should be unified in just one
  • minor optimization: instead of getting multiple times some elements, maybe I'd try to get them just once, see for example how many times .community-events-toggle-location is targeted with jQuery

#21 @azaozz
9 days ago

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

In 40789:

Dashboard:

  • Close the form after obtaining a valid location.
  • Fix focusing the toggle button after closing the form.
  • Fix aria attribute values.
  • Fix positions in IE11.
  • Some JS and CSS cleanup.

Props afercia, coreymckrill.
Fixes #40735.

#22 @afercia
9 days ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening because after the CSS changes the spinner position is now a bit off. Should be an easy fix :)

https://cldup.com/aSrDlKyb0O.png

@iandunn
9 days ago

Restore default Core spinner margin-top, remove padding-bottom

#23 @iandunn
9 days ago

  • Keywords has-patch added; has-screenshots needs-patch removed

40735-spinner.diff restores the normal margin-top: 4px that the spinners have by default. The padding-bottom was removed because it's no longer needed.

@afercia
9 days ago

Adjusts spinner position.

#24 follow-up: @afercia
9 days ago

@iandunn sorry haven't noticed you beat me to it :) in 40735.3.diff the approach is similar, I've just tested a bit also on Windows. Worth noting having a pixel-perfect alignment across platforms is impossible because native fonts have an impact on form field sizes and alignments. I think 40735.3.diff is a good balance.

Last edited 9 days ago by afercia (previous) (diff)

#25 in reply to: ↑ 24 @iandunn
9 days ago

Replying to afercia:

@iandunn sorry haven't noticed you beat me to it :) in 40735.3.diff the approach is similar, I've just tested a bit also on Windows. Worth noting having a pixel-perfect alignment across platforms is impossible because native fonts have an impact on form field sizes and alignments. I think 40735.3.diff is a good balance.

That sounds good, thanks :)

#26 @afercia
9 days ago

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

In 40794:

Dashboard: Improve the Events widget spinner position after [40789].

Props iandunn.
Fixes #40735.

Note: See TracTickets for help on using tickets.