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

AccessibilityManager's handleKey should be triggered by onData, not onKey #5221

Closed
wants to merge 1 commit into from

Conversation

ikicha
Copy link

@ikicha ikicha commented Nov 20, 2024

Issue:
In Android, screen reader feature sometimes generates the voice output including previous content

Reason:
_liveRegion.textContent from previous command is supposed be cleaned up in key stroke event, but onKey doesn't work as intended with IME(except 'enter' in Android) as mentioned in #3025. So the voice output includes previous content.

Fix:
As @Tyriars said in #3025, use onData instead of onKey

@ikicha ikicha changed the title AccessibilityManager's handleKey is triggered by onData, not onKey AccessibilityManager's handleKey should be triggered by onData, not onKey Nov 20, 2024
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.packages.modules.Virtualization that referenced this pull request Nov 27, 2024
With IME, onKey doesn't work well, so xtermjs cannot clean up previous
content properly. So use onData instead.

xtermjs/xterm.js#5221 is the upstream change.

Bug: 376827479
Bug: 376824356
Test: type something + enter, and then type something + space, and then
check talkback behavior.

Change-Id: I6dd2edf8a7aa4d7a29587581b5bd56108bfb7af3
@ikicha
Copy link
Author

ikicha commented Nov 28, 2024

@Tyriar @jerch , can you review this CL?

@ikicha
Copy link
Author

ikicha commented Dec 19, 2024

@Tyriar ping..

@jerch
Copy link
Member

jerch commented Jan 2, 2025

@ikicha Does that still work the same way? Plz note, that onData is special, as it outputs PTY-ready data, which is not necessarily from user input.

Sorry I cannot really review this, as I literally have no clue about accessibility's best practices.

@Tyriar
Copy link
Member

Tyriar commented Jan 6, 2025

We can't use onData here as it is not necessarily user input would therefore make the _charsToConsume part fall over:

https://github.com/anouartouati/xterm.js/blob/c71ef1b91a736db5fad024600f0303809215052d/src/browser/AccessibilityManager.ts#L41-L50

What this change also does is run _handleKey after each and every change to the terminal buffer.

As @Tyriars said in #3025, use onData instead of onKey

This was general advice to embedders of the lib.


Closing since we can't accept this and a different fix would likely look pretty different.

@Tyriar Tyriar closed this Jan 6, 2025
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.

3 participants