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

make the accessibility buffer readonly #4388

Closed
wants to merge 7 commits into from

Conversation

meganrogge
Copy link
Member

@meganrogge meganrogge commented Jan 30, 2023

but still accept arrow keys

172714

@meganrogge meganrogge requested a review from Tyriar January 30, 2023 17:56
@meganrogge meganrogge self-assigned this Jan 30, 2023
@meganrogge meganrogge added this to the 5.2.0 milestone Jan 30, 2023
Comment on lines +133 to +137
if (['ArrowUp', 'ArrowLeft', 'ArrowRight', 'ArrowDown', 'Tab', 'Shift'].includes(event.key)) {
return;
}
event.preventDefault();
event.stopImmediatePropagation();
Copy link
Member

Choose a reason for hiding this comment

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

There are other ways to navigate text (page up, home, etc.). Do we need to prevent writing like this? Restricting it in this way seems more harmful than just allowing editing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have heard from screen reader users in the issue linked and similar that mutating the buffer is unexpected and not desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

By only allowing certain keys, we can make sure that doesn't happen while not messing up the scrolling via the readOnly property

Copy link
Member

@Tyriar Tyriar Jan 31, 2023

Choose a reason for hiding this comment

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

Something I was thinking about recently was that with the move to a textarea, that makes the move to a semantic buffer not possible as you cannot have elements inside a textarea's value. So I think we should go back to a tabindex div with a contenteditable setting (for non-screen readers, until we get F7 caret browsing mode) anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

when it's not content editable, screen readers cannot do anything w a div, but we can do something like this PR does to prevent unwanted edits

Copy link
Member Author

Choose a reason for hiding this comment

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

I also don't think non-screen reader users will gain anything from this unless it is content editable, so we should probably just go with that for everyone.

@meganrogge meganrogge closed this Jan 31, 2023
@Tyriar Tyriar removed this from the 5.2.0 milestone Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants