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

Jump to bottom

A11y tree not working #4571

Closed
JasonXJ opened this issue Jun 20, 2023 · 7 comments · Fixed by #4637
Closed

A11y tree not working #4571

JasonXJ opened this issue Jun 20, 2023 · 7 comments · Fixed by #4637
Assignees
@Tyriar @meganrogge
Labels
area/accessibility type/bug Something is misbehaving
Milestone
5.3.0

Comments

@JasonXJ
Copy link

JasonXJ commented Jun 20, 2023

Details

  • Browser and browser version: Chrome 114 (on linux)
  • xterm.js version: 5.2.1 or ToT

Steps to reproduce

  1. Run the demo
  2. Enable "screenReaderMode"
  3. Check the "xterm-accessibility-tree" element in devtools. All the "listitem" inside are empty. This was working on 5.1.0.

I think we didn't actually bring back the a11y code in here
@martinpitt
Copy link

Confirmed in cockpit-project/cockpit-podman#1358 . We keep xterm.js at 5.1.0 for that reason.

@martinpitt martinpitt mentioned this issue Jul 18, 2023
Merged
martinpitt added a commit to martinpitt/cockpit-podman that referenced this issue Jul 18, 2023
@martinpitt
package.json: Bump most modules
f43e385 
For XTerm 5, use the canvas renderer addon, which was previously built in. The
default renderer uses inline styles and does not work with our strict
C-S-P. Also stop making the Terminal object state. It's completely unnecessary
and may cause unnecessary renders. Adapted from
cockpit-project/cockpit@65471fd9fcb
Don't update to the latest 5.2, as that breaks the a11y tree
(xtermjs/xterm.js#4571). 5.1 works fine.

Note that gettext-parser's current version is 7, but versions ≥ 4 don't
work with our cockpit plugin. That needs to be updated in Cockpit first.
@martinpitt martinpitt mentioned this issue Jul 18, 2023
Closed
martinpitt added a commit to martinpitt/cockpit-machines that referenced this issue Jul 18, 2023
@martinpitt
package.json: Bump most npm modules
6441a9a 
Keep xterm at previous 5.1, as 5.2 breaks the a11y tree
(xtermjs/xterm.js#4571).
This was referenced Jul 18, 2023
Merged
Merged
skobyda pushed a commit to cockpit-project/cockpit-machines that referenced this issue Jul 18, 2023
@martinpitt @skobyda
package.json: Bump most npm modules
4f9ee9e 
Keep xterm at previous 5.1, as 5.2 breaks the a11y tree
(xtermjs/xterm.js#4571).
martinpitt added a commit to martinpitt/cockpit-podman that referenced this issue Jul 18, 2023
@martinpitt
package.json: Bump most modules
2cf5425 
For XTerm 5, use the canvas renderer addon, which was previously built in. The
default renderer uses inline styles and does not work with our strict
C-S-P. Also stop making the Terminal object state. It's completely unnecessary
and may cause unnecessary renders. Adapted from
cockpit-project/cockpit@65471fd9fcb
Don't update to the latest 5.2, as that breaks the a11y tree
(xtermjs/xterm.js#4571). 5.1 works fine.

Note that gettext-parser's current version is 7, but versions ≥ 4 don't
work with our cockpit plugin. That needs to be updated in Cockpit first.
martinpitt added a commit to martinpitt/cockpit-podman that referenced this issue Jul 18, 2023
@martinpitt
package.json: Bump most modules
afd9d6b 
For XTerm 5, use the canvas renderer addon, which was previously built in. The
default renderer uses inline styles and does not work with our strict
C-S-P. Also stop making the Terminal object state. It's completely unnecessary
and may cause unnecessary renders. Adapted from
cockpit-project/cockpit@65471fd9fcb
Don't update to the latest 5.2, as that breaks the a11y tree
(xtermjs/xterm.js#4571). 5.1 works fine.

Note that gettext-parser's current version is 7, but versions ≥ 4 don't
work with our cockpit plugin. That needs to be updated in Cockpit first.
jelly pushed a commit to cockpit-project/cockpit-podman that referenced this issue Jul 19, 2023
@martinpitt @jelly
package.json: Bump most modules
b414dbd 
For XTerm 5, use the canvas renderer addon, which was previously built in. The
default renderer uses inline styles and does not work with our strict
C-S-P. Also stop making the Terminal object state. It's completely unnecessary
and may cause unnecessary renders. Adapted from
cockpit-project/cockpit@65471fd9fcb
Don't update to the latest 5.2, as that breaks the a11y tree
(xtermjs/xterm.js#4571). 5.1 works fine.

Note that gettext-parser's current version is 7, but versions ≥ 4 don't
work with our cockpit plugin. That needs to be updated in Cockpit first.
@Tyriar Tyriar added type/bug Something is misbehaving area/accessibility labels Jul 27, 2023
@Tyriar Tyriar added this to the 5.3.0 milestone Jul 27, 2023
@Tyriar Tyriar self-assigned this Jul 27, 2023
@meganrogge
Copy link
Member

meganrogge commented Jul 31, 2023 •
edited
Loading

Yeah I can see that the live region is a child before I start typing but the rowContainer IE .xterm-accessibility-tree is not ever.

children.mov

@meganrogge
Copy link
Member

each time I type a char, it's hitting this
Screenshot 2023-07-31 at 2 25 38 PM

@meganrogge
Copy link
Member

This'll do it - this._rowContainer is undefined here

Screenshot 2023-07-31 at 2 40 04 PM

@meganrogge
Copy link
Member

maybe that's just dev tools acting weird though 🤔 because it would be throwing if that were undefined

@jerch
Copy link
Member

jerch commented Aug 1, 2023

Yeah setAttribute or access to classList should have thrown here, if it really were undefined.

Hmm its an entry in the "Watch" section, which I dont use. At least the doc states:

The watcher will show you the current value of the variable as it is added.
[...]
The watch list is not a live view of the variables unless you are stepping through execution. [...] To manually recheck the variables in the list press the refresh button to the right of the section heading.

So you prolly need to hit the refresh button to update the values again, if you jumped over a bigger execution stack.

I never had issues with the "Scope" section thought, there should be a this entry under "Local" containing the proper value for _rowContainer right before the breakpoint state. Also these entries are updated automatically.

@meganrogge
Copy link
Member

in a7e0a9c, we changed what the _renderRowsDebouncer does from the left, _renderRows call to _announceCharacters. I think the fix is to bring _renderRows back and still announce chars as we were before

Screenshot 2023-08-02 at 8 09 04 AM

meganrogge added a commit to meganrogge/xterm.js that referenced this issue Aug 2, 2023
@meganrogge
fix xtermjs#4571
33124a3
@meganrogge meganrogge mentioned this issue Aug 2, 2023
Merged
vapier pushed a commit to libapps/libapps-mirror that referenced this issue Jul 7, 2024
terminal: add package xterm-addon-search
d6a4b0f 
We also pin xterm to "~5.1.0" because because newer version suffers from
bug xtermjs/xterm.js#4571.

Bug: b/287604555
Change-Id: I2bab76f9b05a0db955239c68a9b17c9d4a01c56f
Reviewed-on: https://chromium-review.googlesource.com/c/apps/libapps/+/4653820
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Tyriar Tyriar

@meganrogge meganrogge

Labels
area/accessibility type/bug Something is misbehaving
Projects
None yet
Milestone
5.3.0
Development

Successfully merging a pull request may close this issue.

fix a11y tree meganrogge/xterm.js
5 participants
@martinpitt @Tyriar @jerch @JasonXJ @meganrogge