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

[Editor] Make FreeText annotations visible for screen readers when in editing mode (bug 1793419) #15595

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

calixteman
Copy link
Contributor

  • When we're editing some annotations, keeping the role="text-box" make them visible as editable and VoiceOver (Mac) is able to read the contents when they're focused;
  • Add an attribute "aria-activedescendant" in order to make the content discoverable by NVDA on Windows.

… editing mode (bug 1793419)

- When we're editing some annotations, keeping the role="text-box" make them visible
as editable and VoiceOver (Mac) is able to read the contents when they're focused;
- Add an attribute "aria-activedescendant" in order to make the content discoverable
by NVDA on Windows.
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Given my complete lack of a11y knowledge I cannot really review this as such, however the actual code changes seem OK as far as I can tell.

@calixteman
Copy link
Contributor Author

I tested on Mac 12.6 with VoiceOver and on Windows 11 with NVDA, in both cases I think it works as expected (I'm not an a11y expert either).

@calixteman
Copy link
Contributor Author

Anyway, I pinged @MReschenberg about this patch.

@MReschenberg
Copy link

MReschenberg commented Oct 19, 2022

Hihi, I've tested this locally -- looks good to me :)
@calixteman I'm testing on 12.5.1 and on my machine (for some reason) the VO visual cursor doesn't accurately bound the annotation. Instead of tightly wrapping the text editor area, the VO visual cursor is off to the side. VO correctly reads/routes actions to the editor object though, and the editor text seems to be "in flow" with its location in the PDF.

Are you seeing anything similar on your end?
I don't think this is a result of this patch (I can repro in Nightly) but thought I'd flag it.

@calixteman
Copy link
Contributor Author

@MReschenberg, could you attach a screenshot ? I used VO for the 1st time today, so I'm not sure to know what it is correct or not.

@MReschenberg
Copy link

image

yep here ya go, the black rectangle box is the VO visual cursor. Even though it doesn't match up with where the annotation area actually is, that's what it says is focused.

@calixteman
Copy link
Contributor Author

I'm not able to reproduce in neither nightly nor release (and even not in a local build with my patch), but my macOS version is 12.6. Anyway, as you said it's unrelated to this patch, so let's land it!

@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/147311d408e1c9c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/6507d2f089b4a97/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/147311d408e1c9c/output.txt

Total script time: 3.82 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/6507d2f089b4a97/output.txt

Total script time: 11.98 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor Author

Windows integration tests are green locally: I really need to fix them on the bot.

@calixteman calixteman merged commit ba3a0e1 into mozilla:master Oct 19, 2022
@calixteman calixteman deleted the 1793419 branch October 19, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants