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

Change confirm action to dialog element #2357

Closed
wants to merge 5 commits into from

Conversation

krvpb024
Copy link

@krvpb024 krvpb024 commented Feb 12, 2024

This pull request is the last that will change the current UI in order to improve accessibility. The rest might be some bug fixes if they're reported.

A screenshot of the comparison between the current and the pull request.

Purpose of this pull request

The current confirmation dialog approach may cause some inconvenience to some screen readers.

Completely lost focus position

The current approach will hide the trigger button, and some screen readers (Android TalkBack, macOS VoiceOver) will lose the focus position and go back to the top of the web page.

Without enough information about what the current state is

Linux Orca's screen reader position was lost, and it went back to the top of the web page. But it remained in the keyboard focus position. If the user presses the TAB key, the next focus item will be the yes button. Windows NVDA remained the focus position, both in the screen reader and keyboard focus positions, but it will stay silent. Either behavior doesn't announce there is a dialog waiting for user action. This will confuse screen reader users, because they might think this action is already in progress, then wait for the screen reader to announce whether the page is reloaded or the success message is shown. Until the user tries to navigate to the next element, they will finally realize they have to resolve the confirmation dialog.

How to improve this

By replacing the current approach with HTML dialog.

Some benefits

Some behaviors HTML dialog provide can reduce the size of JavaScript, in order to implement them.

Autofocus on the dialog element

The dialog element has a clear focusing step. Screen readers will have a clear announcement of the current focusing element, which is in an alert dialog, so users will be informed the alert dialog is waiting for their input. And when the user closes the dialog, browsers will handle the focus back to the button that triggers the dialog show.

The inert behavior

Inert on elements other than dialog can force the user to dismiss the dialog first. This can prevent users from accidentally leaving the confirmation dialog and having to find a way back.


Do you follow the guidelines?

@krvpb024 krvpb024 marked this pull request as draft February 12, 2024 03:06
@krvpb024 krvpb024 marked this pull request as ready for review February 12, 2024 06:56
Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

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

I haven't tested all the views yet, but the dialog on the API keys, Edit Feed and Users pages is broken:

image

The text in the dialog could be confusing for the end-user. I would move the dialog title to the top, next to the icon to close the window.

image

In the future, perhaps the text could be improved to add more context in the popup. In this example above, the end-user doesn't really know what feed is going to be removed.

</svg>
</button>
</form>
<button class="alert-dialog-confirm-button button button-danger">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the primary button should have the class button-danger, button-primary looks more appropriate to me. The secondary button could have only the class button or a new class button-secondary.

Another possibility would be to make it configurable depending on the type of action. For example, deleting something can be button-danger, but refreshing a feed is a button-primary.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe .button-primary for yes button, .button or .button_secondary for no. And if target button has data-is-danger then classList would be .button-primary .button-danger.

@krvpb024
Copy link
Author

krvpb024 commented Feb 13, 2024

So it may look like this?

Title: Remove this feed | Close button
if has Description: show which feed is about to remove
Yes button | No button

@krvpb024
Copy link
Author

krvpb024 commented Feb 14, 2024

test layout

I've changed the dialog layout according to your suggestions. If you think it's OK, then I'll start to implement it.

@krvpb024
Copy link
Author

Dialog has action target in context:

  • Unshare entry
  • Remove category
  • Remove feed
  • Remove user
  • Mark all as read to feed item in /feeds page
  • Mark all as read to category item in /categories page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants