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

[Accesibility: spaces] Switching to another space should show a notification #36746

Closed
wants to merge 10 commits into from

Conversation

PhilippBaranovskiy
Copy link
Contributor

@PhilippBaranovskiy PhilippBaranovskiy commented May 21, 2019

Related: #23885

Summary

Here I did two things:

  1. Filled context item in switching space popover with aria-label.
  2. Added a showing a toast notification with delayed page reload.

Then: clicking the switch space trigger immediate page reload.
Now: it shows a notification, waits 4s for reloading the page.

This is one of the 3 steps to improve that behaviour:

1. Make notifications showing while switching to another space: #36746
2. Make notification announced by a ScreenReader: needs wait until elastic/eui#2055 is merged into Kibana.
3. Make showable notification hidden from a visual layout (ScreenReaderOnly)

Additional steps

  • Add an option to make toast notification screen reader only in EUI
  • Wait until that fix is merged with EUI update in Kibana
  • Add a fix using prev line in Kibana

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@PhilippBaranovskiy PhilippBaranovskiy added the WIP Work in progress label May 21, 2019
@PhilippBaranovskiy PhilippBaranovskiy self-assigned this May 21, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@PhilippBaranovskiy PhilippBaranovskiy changed the title WIP: Screenreader should announce that user is in a new space #23885 [Accesibility: spaces] Switching to another space should show a notification Jun 20, 2019
@PhilippBaranovskiy PhilippBaranovskiy removed the WIP Work in progress label Jun 20, 2019
@PhilippBaranovskiy PhilippBaranovskiy marked this pull request as ready for review June 20, 2019 13:08
@PhilippBaranovskiy PhilippBaranovskiy requested a review from a team as a code owner June 20, 2019 13:08
@PhilippBaranovskiy PhilippBaranovskiy added Feature:Security/Spaces Platform Security - Spaces feature Project:Accessibility Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.3.0 WCAG A labels Jun 20, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

onClose: () => this.props.spacesManager.changeSelectedSpace(space),
});

setTimeout(() => this.props.spacesManager.changeSelectedSpace(space), 4000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching between spaces today already takes a decent amount of time because of the full page reload that's required, and the fact that we place users on the Kibana home screen following a space switch. So it likely takes users at least 10 seconds to switch spaces and navigate to the application they're interested in.

I feel that adding an additional 4 seconds before any of that happens would unnecessarily frustrate a large number of users. Now that the space selector menu has a descriptive aria-label ("Switch to {spaceName} space"), do we need this additional notification? We don't do this dual-notification when switching between applications, for example.

IF we do need this notification, then the toast is rather hard to discover. This is perhaps more of a question for the design team, but switching a space happens in the top left of the screen, but the notification appears in the bottom right of the screen:
image

Copy link
Contributor Author

@PhilippBaranovskiy PhilippBaranovskiy Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legrego this is not going to be visible in the future, however, making toast notifications able to hidden but announced is another PR: elastic/eui#2055

Delay you mentioned is crucial indeed.
I think you are right that we have already aria-label before hit switching, so the main deal here is to confirm for a user that switching process is starting.
Would it be enough to announce "Switching..." and reload the page? What do you think?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@PhilippBaranovskiy
Copy link
Contributor Author

PhilippBaranovskiy commented Jun 26, 2019

retest

@PhilippBaranovskiy
Copy link
Contributor Author

@legrego, could you please have a look and help take the final decision?
Also, as you remember, all of us discussed the possible solutions here: #23885
and got agreed that showing a notification, wait for reading out and reload the page may be a temporary solution for a while.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@PhilippBaranovskiy
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member

legrego commented Jun 26, 2019

could you please have a look and help take the final decision?

Sorry for the delay, I'm not on my normal duties this week, so my time has been rather limited.

this is not going to be visible in the future, however, making toast notifications able to hidden but announced is another PR: elastic/eui#2055

It looks like the EUI upgrade which includes elastic/eui#2055 is in progress here: #39601. Once that merges, can we update this PR to use the hidden toasts, rather than following this up with another PR to fix the behavior?

Delay you mentioned is crucial indeed.
I think you are right that we have already aria-label before hit switching, so the main deal here is to confirm for a user that switching process is starting.
Would it be enough to announce "Switching..." and reload the page? What do you think?

I agree it'd be enough to simply announce "switching", or maybe "switching space". What do you think is the absolute minimum of a delay we can introduce? I'd like for this to not impact users who do not need a screen reader.

Since this process requires a call to /api/spaces/v1/space/{id}/select before proceeding, I wonder if we could use the delay introduced there to our advantage. Something like:

  1. Announce "Switching space"
  2. Make API request to /api/spaces/v1/space/{id}/select via spacesManager.changeSelectedSpace()
  3. If API request completes after out defined wait period, then simply perform the redirect. Otherwise, delay for the remainder of the wait period, and redirect at that point.

Since we're introducing a delay, I think it'd make sense to give a visual indication in the space selector itself that the operation is in progress. Maybe a spinner of some sort, but I'll defer to @elastic/kibana-design on that. On resource constrained installations, there is already a perceivable delay when switching spaces, so I worry if we make that worse that users will think that Kibana isn't responding to them.

@PhilippBaranovskiy
Copy link
Contributor Author

Once that merges, can we update this PR to use the hidden toasts, rather than following this up with another PR to fix the behavior?

Yes, we can. Let's do so.

@PhilippBaranovskiy
Copy link
Contributor Author

It looks like the EUI upgrade which includes elastic/eui#2055 is in progress here: #39601.

Not exactly as it looks. That PR 2055 is about another issue, so now it needs to add hidden toasts in EUI and then come here up to update this PR.

@PhilippBaranovskiy
Copy link
Contributor Author

I agree it'd be enough to simply announce "switching", or maybe "switching space".

I have to admit that I was not right. The main issue here is that we should make a user sure that they are getting switched to the chosen space. Thus we should announce the space name.

Thus the shortest option is "Switching to {name} name."

@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego
Copy link
Member

legrego commented Jun 27, 2019

I have to admit that I was not right. The main issue here is that we should make a user sure that they are getting switched to the chosen space. Thus we should announce the space name.

I think this is the part that I'm having a hard time understanding. When users switch between applications in Kibana, we don't have to announce that in any special way. We just let the click the link and off they go. Switching applications has the potential to be as much of a context switch as changing spaces, so I don't quite understand why spaces needs all of this special treatment.
If they've navigated to a specific space in the space selector, and they choose to click that space, it works like any other link, right?

If the window title were updated to include the current space name, would that provide enough context, or is the title not always announced on page reloads?

@bhavyarm
Copy link
Contributor

@myasonik @barlowm can you please take a look at this and this issue and let us know what you think about it? #23885 especially Larry's comment above ^ . Thanks!

@PhilippBaranovskiy
Copy link
Contributor Author

If they've navigated to a specific space in the space selector, and they choose to click that space, it works like any other link, right?
If the window title were updated to include the current space name, would that provide enough context, or is the title not always announced on page reloads?

These are good questions, I'm not sure about the most correct solution here.
Let's wait for someone's else opinion as Bhavya suggested.

@myasonik
Copy link
Contributor

myasonik commented Jun 28, 2019

I do think this PR helps the situation but I think we can accomplish more in other ways without having to rely on artificial delays and visually hidden toasts. Ultimately, with all of the other improvements that can be made, I don't think any sort of custom notification is required for this.

I was going to try to enumerate* all of the bugs and improvements that can be made but it became difficult because, I believe, the component powering the dropdown and the items within is inherently the wrong abstraction and has poor semantics for this case.

There's currently a draft of an upcoming "disclosure pattern" for dropdown navigation which I think is a much better fit for this component. Building on top of that, with maybe a couple more descriptive elements, I think we can achieve everything we've set out to do.

If it'd be helpful I can relatively quickly build a demo of what the final experience could be like in codepen.


* Ok, I will try enumerate the issues after all for the sake of education and discussion:

  1. All items should have a focus state. We loose a visual indicator for focus when it's on the popover panel.

    1.a. There is a wrapping element which takes focus around the whole popover which also does not have a visual focus indicator and serves no apparent role but does cause confusion ("why am I tabbing but it not change anything?").

  2. In the dropdown, all of the buttons should be links. Generally, if it changes the URL, it should be a link. I can see an argument being made for it being a button because of the changing backing data but I think a link can communicate that just as well while also communicating that there will be a full page refresh.

  3. The "open popup" button has the attribute aria-haspopup="true" which strictly means it opens a menu however none of the opened content is of role="menu". A literal role="menu" is also likely not what we want on the dropdown so we should remove the haspopup attribute all together.

  4. The items within the the popup have odd keyboard functionality. It seems like it's half way to implementing some list semantics but also misses many. A big part of accessible interfaces relies on consistent patterns and this is kind of in the uncanny valley of close in some ways but not quite.

  5. The items within the popup notably is missing the last item.

@barlowm
Copy link
Collaborator

barlowm commented Jul 1, 2019

Apologies for not getting back to this sooner.
@myasonik good points in your comment, though in looking through WCAG - 3.2.2 - On Input - to specifically address the issue here,

Changing the setting of any user interface component does not automatically
cause a change of context unless the user has been advised of the behavior 
before using the component.

There was a note that

Note: A change of content is not always a change of context. 
This success criterion is automatically met if changes in content are not also 
changes of context.

Now with all that being said, one would think that any link which opens a new page should alert the user to a change in context. However in navigating through sites like WebAim, I don't see any link that goes to a new page reporting a change in context. I think that most web users understand that a link will take you someplace else (the exceptions are when a link opens a new window/tab which would change the users back/forward navigation ability).
So in this context I don't believe that changing to another space would need to show a notification.

  1. In the dropdown, all of the buttons should be links. Generally, if it changes the URL, it should be a link. I can see an argument being made for it being a button because of the changing backing data but I think a link can communicate that just as well while also communicating that there will be a full page refresh.

I agree, and have always had that preference as well. But that's more of a design choice than a WCAG issue, and so long as it's consistent throughout the entire application it should not be an issue.

  1. The "open popup" button has the attribute aria-haspopup="true" which strictly means it opens a menu however none of the opened content is of role="menu". A literal role="menu" is also likely not what we want on the dropdown so we should remove the haspopup attribute all together.

The aria-haspopup indicates the availability of a menu or dialog, and possibly rather than simply having a value of "true" perhaps it should have a value of "listbox" or "menu" (aria-haspopup="listbox" aria-haspopup="menu") then the `role="menu" would be supurflous.

  1. The items within the the popup have odd keyboard functionality. It seems like it's half way to implementing some list semantics but also misses many. A big part of accessible interfaces relies on consistent patterns and this is kind of in the uncanny valley of close in some ways but not quite.

Correct, this is the entire point behind 3.2.3 Consistent Navigation - Level AA

@spalger spalger added the v7.4.0 label Jul 3, 2019
@myasonik
Copy link
Contributor

myasonik commented Jul 8, 2019

Closing this PR with agreement from @bhavyarm and @barlowm. Plan to address underlying a11y issue:

  • fix dropdown screen reader issues (likely, in EUI)
  • change buttons to links

Importantly, this avoids the issue of artificial delays to show/announce notifications.

@myasonik myasonik closed this Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Spaces Platform Security - Spaces feature Project:Accessibility release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.3.0 v7.4.0 WCAG A
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants