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

fix urls with screenreader #4491

Merged
merged 1 commit into from
May 3, 2023
Merged

fix urls with screenreader #4491

merged 1 commit into from
May 3, 2023

Conversation

jerch
Copy link
Member

@jerch jerch commented Apr 24, 2023

@Tyriar This small change fixes things for the url event listeners. Idk if it will break some functionality on screenreader side, thus it furthermore needs proper testing with a screenreader app being attached.

@Tyriar
Copy link
Member

Tyriar commented May 2, 2023

@meganrogge could you try this out? Previously pointer-events weren't going through beyond the a11y dom tree in order for the text outlines to work correctly by hovering with a mouse for low-vision users. I'm not sure this is still a problem?

@Tyriar Tyriar requested a review from meganrogge May 2, 2023 18:21
@meganrogge
Copy link
Member

meganrogge commented May 3, 2023

Before the change vs after the change 👍🏼

Using VoiceOver with this setting, I was hoping that the link would get announced, but it doesn't. It did not before the change either.

Screenshot 2023-05-03 at 8 15 30 AM

vo.mov

@Tyriar Tyriar added this to the 5.2.0 milestone May 3, 2023
@Tyriar Tyriar merged commit 913cb25 into xtermjs:master May 3, 2023
@jerch
Copy link
Member Author

jerch commented May 5, 2023

... I was hoping that the link would get announced, but it doesn't. ...

I think this highly depends on what underlying data structure/event the screenreader will trigger its link announcement. This is prolly DOM link related, but while we visually mimick a DOM link here, the underlying data is still nowhere close to a DOM link firing its hover event. But it might be possible to fake that as well in the aria container. Maybe just attach a link provider listener, that places a DOM link and triggers a hover in the aria element will already do? (Have lit. no clue about aria events, so this is just a wild guess...)

@Tyriar Tyriar mentioned this pull request May 7, 2023
@Tyriar
Copy link
Member

Tyriar commented May 7, 2023

@meganrogge it occurs to me now we probably shouldn't have ripped out the a11y tree considering we moved the accessible buffer feature into vscode's codebase #4503

@RahjIII
Copy link

RahjIII commented Aug 3, 2023

Thanks for the fix. Verified in xterm 5.2.1.

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.

4 participants