Events widget form should close after successful search

[iandunn]

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

[afercia]

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...

[iandunn]

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.

[iandunn]

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.

[afercia]

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.

[obenland]

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.

[afercia]

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]

Removes unneeded class to avoid unwanted background on click

[iandunn]

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​.

[iandunn]

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.

[afercia]

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.

[coreymckrill]

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

[iandunn]

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.

[afercia]

@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:

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?

[afercia]

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.

[afercia]

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

[afercia]

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]

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.

[iandunn]

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]

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

[azaozz]

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.

[afercia]

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

[iandunn]

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

[iandunn]

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]

Adjusts spinner position.

[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.

[iandunn]

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 :)

[afercia]

In 40794:

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

Props iandunn.
Fixes #40735.