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

Implement StructTree, improve accessibility #1498

Merged
merged 12 commits into from
Jun 6, 2023

Conversation

MattL75
Copy link
Contributor

@MattL75 MattL75 commented May 10, 2023

@wojtekmaj Here is my rough proof-of-concept. I am having trouble getting Vitest to run. It seems to hang forever so I cannot run nor write unit tests at the moment :/

Adds accessible semantics to the canvas. Screen readers can now get great information for semantic elements such as lists.

Tested to work with Voiceover on mac.

@MattL75 MattL75 changed the title feat: 1494 Adds accessible semantics draft: feat: 1494 Adds accessible semantics May 10, 2023
@MattL75 MattL75 marked this pull request as draft May 11, 2023 13:10
@MattL75
Copy link
Contributor Author

MattL75 commented May 11, 2023

Still unsure why but downgrading Vitest to 0.28.5 allows me to run the tests.

@MattL75 MattL75 force-pushed the feat/1494/add-accessibility branch from 821fc8b to 8a3f437 Compare May 11, 2023 19:06
@MattL75
Copy link
Contributor Author

MattL75 commented May 11, 2023

@wojtekmaj The only concern I have with this is the custom text renderer. There is no longer a direct mapping between textContent and the dom.

We could call getTextContent with the includeMarkedContent argument and only allow customization of the items which have a string attached.

What are your thoughts on this?

One option would be to look for the matching text in the DOM, though that would be much slower than the current implementation.

@MattL75 MattL75 force-pushed the feat/1494/add-accessibility branch from 8a3f437 to 31db4e3 Compare May 12, 2023 17:14
expect(textItems).toHaveLength(desiredTextItems.length + 1);
});

it('maps textContent items to actual TextLayer children properly', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I feel this test is pointless now as there is no longer a direct mapping between customTextRender and normal.

@@ -184,7 +184,7 @@ export default function TextLayer() {

layer.innerHTML = '';

const textContentSource = page.streamTextContent();
const textContentSource = page.streamTextContent({ includeMarkedContent: !customTextRenderer });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solution to the customTextRenderer problem. No change of behaviour if you use a custom render function.

Copy link
Owner

@wojtekmaj wojtekmaj Jun 5, 2023

Choose a reason for hiding this comment

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

What's "customTextRenderer problem"? The fact one could render text items and remove some properties in the process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well previously the custom text renderer had a 1-to-1 ratio of elements to text content. Now it's a bit different as the structure has more nested stuff within. I think the safest option is to just disable the accessibility layer if there is a custom renderer

@MattL75 MattL75 force-pushed the feat/1494/add-accessibility branch from 31db4e3 to 19a52b7 Compare May 12, 2023 19:10
@MattL75 MattL75 marked this pull request as ready for review May 12, 2023 19:11
@MattL75 MattL75 changed the title draft: feat: 1494 Adds accessible semantics feat: #1494 Adds accessible semantics May 12, 2023
@MattL75
Copy link
Contributor Author

MattL75 commented May 24, 2023

@wojtekmaj Any thoughts on this? :)

src/StructTree/types.ts Outdated Show resolved Hide resolved
@wojtekmaj wojtekmaj force-pushed the feat/1494/add-accessibility branch from a2d7164 to df1e2c8 Compare June 5, 2023 21:38
@wojtekmaj
Copy link
Owner

I refactored the code a bit to ensure type safety and to use useResolver which allows me to handle async loading more gracefully. I'm still not certain about a few things, please see my comments :)

@wojtekmaj wojtekmaj added the enhancement New feature or request label Jun 5, 2023
@wojtekmaj wojtekmaj force-pushed the feat/1494/add-accessibility branch from 8af7185 to 1666dae Compare June 5, 2023 22:03
@wojtekmaj
Copy link
Owner

wojtekmaj commented Jun 6, 2023

TODO:

  • Grow some brain and understand the includeMarkedContent/customTextRenderer issue deeply
  • Document new callbacks
  • Re-add relevant unit tests
  • Figure out if StructTree is safe to render without text layer

@wojtekmaj wojtekmaj changed the title feat: #1494 Adds accessible semantics Implement StructTree, improve accessibility Jun 6, 2023
@MattL75
Copy link
Contributor Author

MattL75 commented Jun 6, 2023

imo rendering struct tree without text layer is a bit pointless anyways. It could add a bit of context to some stuff like images. Honestly though if you're disabling the text layer you probably don't care much about making the document accessible

@wojtekmaj
Copy link
Owner

Not really about "care"/"not care" - but e.g. we have incoming #1519 :)

@wojtekmaj wojtekmaj force-pushed the feat/1494/add-accessibility branch from bcf9c37 to 5a14f71 Compare June 6, 2023 13:27
@wojtekmaj wojtekmaj force-pushed the feat/1494/add-accessibility branch from aca8fa4 to ec57559 Compare June 6, 2023 13:52
@wojtekmaj
Copy link
Owner

wojtekmaj commented Jun 6, 2023

Phew, made it work in all cases, brought back the tests, everything looks pretty good in my opinion :D I know it's silly but can I ask YOU for a review now? 😂 Especially the screen reader with and without custom text renderer.

@wojtekmaj
Copy link
Owner

Let's goooo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants