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.

Sign up for GitHub

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
Closed

make the accessibility buffer readonly #4388

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3620f57
test
meganrogge Jan 26, 2023
4cf2523
test 2
meganrogge Jan 26, 2023
a993894
testing 3
meganrogge Jan 26, 2023
8c10fda
revert
meganrogge Jan 26, 2023
48ec328
make readonly
meganrogge Jan 30, 2023
cdab11e
lint
meganrogge Jan 30, 2023
0711ae4
Update src/browser/AccessibilityManager.ts
meganrogge Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/browser/AccessibilityManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ export class AccessibilityManager extends Disposable {
this._accessiblityBuffer.ariaLabel = Strings.accessibilityBuffer;
this._accessiblityBuffer.classList.add('xterm-accessibility-buffer');
this._accessiblityBuffer.addEventListener('focus', () => this._refreshAccessibilityBuffer());
this._accessiblityBuffer.addEventListener('keydown', (e) => this._handleAccessibilityBufferKey(e));
this._accessiblityBuffer.addEventListener('keyup', (e) => this._handleAccessibilityBufferKey(e));
this._accessiblityBuffer.addEventListener('keypress', (e) => this._handleAccessibilityBufferKey(e));
this._accessibilityTreeRoot.appendChild(this._accessiblityBuffer);

this.register(this._renderRowsDebouncer);
Expand Down Expand Up @@ -126,6 +129,14 @@ export class AccessibilityManager extends Disposable {
}));
}

private _handleAccessibilityBufferKey(event: KeyboardEvent): void {
if (['ArrowUp', 'ArrowLeft', 'ArrowRight', 'ArrowDown', 'Tab', 'Shift'].includes(event.key)) {
return;
}
event.preventDefault();
event.stopImmediatePropagation();
Comment on lines +133 to +137
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 •
edited
Loading

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.

}

private _handleBoundaryFocus(e: FocusEvent, position: BoundaryPosition): void {
const boundaryElement = e.target as HTMLElement;
const beforeBoundaryElement = this._rowElements[position === BoundaryPosition.TOP ? 1 : this._rowElements.length - 2];
Expand Down