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

add accessibility buffer #171118

Merged
merged 36 commits into from Jan 19, 2023
Merged

add accessibility buffer #171118

merged 36 commits into from Jan 19, 2023

Conversation

meganrogge
Copy link
Contributor

fixes #169853

@meganrogge meganrogge marked this pull request as ready for review January 18, 2023 18:38
@Tyriar
Copy link
Member

Tyriar commented Jan 19, 2023

I notice mouse wheel events still skip the a11y buffer and go to the main terminal, ideally we would change that as well as when you click on it to not switch back to the main terminal view.

@meganrogge
Copy link
Contributor Author

I notice mouse wheel events still skip the a11y buffer and go to the main terminal, ideally we would change that as well as when you click on it to not switch back to the main terminal view.

Oh, I thought that was by design as I thought ppl would be using the keyboard to navigate and mouse to escape

@meganrogge meganrogge changed the title add accessibility element to the terminal add accessibility buffer Jan 19, 2023
@Tyriar
Copy link
Member

Tyriar commented Jan 19, 2023

Oh, I thought that was by design as I thought ppl would be using the keyboard to navigate and mouse to escape

It was, but that was before I realized it would eventually enable keyboard caret selection. We can skip it for now as it's only on when you enable a11y mode. There are also low-vision users who may use screen readers and a mouse, so ideally it would work.

return;
}
// The viewport is undefined when this is focused, so we cannot get the cell height from that. Instead, estimate using the font.
const cellHeight = this._configHelper.getFont((this.xterm.raw as any)._core)?.charHeight || undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Can you use this.xterm.getFont() instead?

}
// The viewport is undefined when this is focused, so we cannot get the cell height from that. Instead, estimate using the font.
const cellHeight = this._configHelper.getFont((this.xterm.raw as any)._core)?.charHeight || undefined;
this._accessibilityBuffer.style.lineHeight = cellHeight ? (this._configurationService.getValue(TerminalSettingId.LineHeight) as number) * cellHeight + 'px' : '';
Copy link
Member

Choose a reason for hiding this comment

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

getFont should return the lineHeight as well?

@meganrogge meganrogge merged commit ef9fb8a into main Jan 19, 2023
@meganrogge meganrogge deleted the merogge/xterm-acc branch January 19, 2023 18:58
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore plain content editable element for terminal buffer instead of navigation mode
2 participants