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 a Time-Based Debouncer to update Screen Readers at most once per second #3399

Merged
merged 11 commits into from
Aug 18, 2021

Conversation

pattch
Copy link
Contributor

@pattch pattch commented Jul 29, 2021

This change makes it so that screen readers are announced to based on the most recent update to Screen Readers, as opposed to every possible animation frame. Previously, the very frequent updates to screenreaders in conjunction with the unbounded internal character buffer for announcements caused full page crashes due to out of memory errors when screen reader refreshes weren't fast enough to keep up with fast updates to Terminals. This change ensures that Screen Readers are refreshed at most once per second, but preserves the behavior for visual Terminal refreshes by introducing a second debouncer based on the existing RenderDebouncer.

Closes #3374

@pattch
Copy link
Contributor Author

pattch commented Jul 31, 2021

Note: this pull request DOESN'T trim any content from the Accessibility Manager's internal character buffer, as was previously intended. In practice, throttling the frequency of updates is sufficient in practice to prevent the screen reader updates from causing the crashes as before. As a precaution, I still think the internal buffer should still have a cap on the size it can grow to, but that's not addressed in this PR.

Closes #3376

@pattch
Copy link
Contributor Author

pattch commented Aug 16, 2021

Just wanted to double check and see if there is any update here, and see whether this change is an acceptable fix for xterm.js?

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Sorry about the delay, approach looks good overall 👍

src/browser/TimeBasedDebouncer.ts Outdated Show resolved Hide resolved
src/browser/TimeBasedDebouncer.ts Outdated Show resolved Hide resolved
src/browser/TimeBasedDebouncer.ts Show resolved Hide resolved
@pattch
Copy link
Contributor Author

pattch commented Aug 17, 2021

No worries on the delay - thanks for the constructive feedback! I've done my best to address the issues you brought up, and so this PR should now conform to those expectations. Let me know if there's anything else missing from this fix, thanks

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Perfect, thanks! CI failure is a flake

@Tyriar Tyriar merged commit 9490c5c into xtermjs:master Aug 18, 2021
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.

[Google Cloud Shell] Screen Reader causing full page crashes when terminal is updated frequently.
2 participants