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

Refactor: Rewrite of the Explorer to use ARIA trees #987

Closed
wants to merge 68 commits into from

Conversation

zorkow
Copy link
Member

@zorkow zorkow commented Aug 14, 2023

This is the long awaited rewrite of the Explorer to use the ARIA tree paradigm.

  • Speech (and Braille) is compute during enrichment and set in the appropriate aria-label or aria-braillelabel. Consequently each element has four additonal attributes:
    • data-semantic-speech contains speech computed by SRE in ssml markup.
    • data-semantic-braille contains the braille computed by SRE (this could be in markup as well in the future, e.g., for 2D output).
    • aria-label with the speech without ssml markup
    • aria-braillelabel with the braille without markup
  • Navigation is now handled directly in the KeyExplorer that also handles all subtitle and magnification regions.
  • Explorer is started with Enter or mouse click inside the structure.
  • Functionality ported from the old explorer so far:
    • Arrow key navigation
    • Triggering hyperlinks with Enter
    • Self-voicing and synchronised highlighting
  • Adds a SpeechUtil module for some of the functionality related to speech computation.

Note, this works with the current version of SRE.

Known Issues and Next Steps

  • Self-voicing of leaf expressions can cause highlighting bugs when we move to quickly between expressions. Second highlighter (red) works concurrently and is triggered once the voicing stops. Thus the first highlighter (light blue) might fire first and therefore will not unhighlight a node, when it is red.
  • Port other speech functionality from SRE. However some of that needs changes in SRE. This PR works with the current version of SRE.
  • We do not recompute the semantic structure in SRE after enrichment.

dpvc and others added 30 commits September 20, 2022 21:47
@zorkow zorkow requested a review from dpvc August 14, 2023 17:10
@zorkow zorkow marked this pull request as ready for review August 14, 2023 17:11
Copy link
Member

@dpvc dpvc left a comment

Choose a reason for hiding this comment

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

This looks good over all, but I'm concerned about one thing. The setAria() function uses buildSpeech(), which in turn uses ssmlParsing(), which uses DOMParser. That is only available in the browser, so that may cause problems when semantic-enrich is used in node, though I haven't actually tried it out.

It may be helpful to put setAria() behind an option that disables it for use in node, though it might be nice to have it work there as well. For that, you would need to use the document adaptor to parse and manipulate the XML. There are methods for parsing an XML string, and you could use adaptor.kind() as a substitute for node.nodeType (sort of). So it looks like it would be possible to make the speech generation even in node. Of course, the explorer will remain browser-dependent, but it would be nice for semantic-enrich to work for node use.

ts/a11y/SpeechUtil.ts Show resolved Hide resolved
ts/a11y/SpeechUtil.ts Outdated Show resolved Hide resolved
ts/a11y/SpeechUtil.ts Outdated Show resolved Hide resolved
ts/a11y/SpeechUtil.ts Show resolved Hide resolved
ts/a11y/SpeechUtil.ts Outdated Show resolved Hide resolved
ts/a11y/explorer/KeyExplorer.ts Show resolved Hide resolved
ts/a11y/explorer/KeyExplorer.ts Outdated Show resolved Hide resolved
ts/output/chtml.ts Show resolved Hide resolved
ts/a11y/semantic-enrich.ts Outdated Show resolved Hide resolved
ts/a11y/semantic-enrich.ts Outdated Show resolved Hide resolved
@zorkow
Copy link
Member Author

zorkow commented Sep 26, 2023

This looks good over all, but I'm concerned about one thing. The setAria() function uses buildSpeech(), which in turn uses ssmlParsing(), which uses DOMParser. That is only available in the browser, so that may cause problems when semantic-enrich is used in node, though I haven't actually tried it out.

I can use the parsing method from SRE, which selects the correct DomParser either from the browser or the xmldom library.

@dpvc dpvc added this to the v4.0 milestone Nov 6, 2023
@zorkow zorkow changed the base branch from sre_latex_braille_ext to develop December 6, 2023 18:33
@zorkow
Copy link
Member Author

zorkow commented Dec 6, 2023

@dpvc I did the review requests. In particular, the DOMparser is now the SRE one, which chooses the correct one depending on the environment.
The PR is already approved, but just in case you want to have another look, I will not merge it directly.

I've also rebased from the original no longer existing branch and merged develop.

@dpvc
Copy link
Member

dpvc commented Dec 9, 2023

I'm good with this, other than two things:

  1. I'm wondering if clicking to enter the walker is a good idea, as I think that will be unexpected by most people. For example, I know there are people who drag select sections of the math to copy and paste it, and this might be confusing to them (or interfere with that ability).

  2. The fact that this branch incorporates the old v4.0-alpha branch, which is not part of the current develop branch, and was not intended to be merged into develop, is a problem. You are getting a lot of extra commits that shouldn't be be there. I will look into making a clean branch that cherry-picks the needed commits from this one, while leaving out the v4.0-alpha commits, and see if I can't straighten that out like I did once before on another branch that was based on the alpha branch. This one will be a bit trickier, though.

dpvc pushed a commit that referenced this pull request Dec 9, 2023
@dpvc
Copy link
Member

dpvc commented Dec 9, 2023

I have made a fresh branch explorer-rewrite-tmp (with dashes rather than underscores) that is based of the current develop branch, and cherry-picked the commits from this PR that were not already in the develop branch. That gets rid of all the v4.0-alpha noise, and makes this a clean branch without a lot of merging in of other branches along the way. The command git diff explorer-rewrite-tmp explorer_rewrite_tmp shows no differences, so I think everything is as it should be. I recommend we close this PR and open a new one for explorer-rewrite-tmp and make any other changes from there.

If there are other branched based off v4.0-alpha, they need to get a similar treatment. These seem to be

  • refactor/speechgenerator_pool
  • complete_explorer_functionality
  • sre_latex_braille_ext
  • explorer_rewrite_euro

The last one is just a single commit from 2cb1708, so easily recreated from the new explorer-rewrite-tmp without the v4.0-alpha dependency. The sre_latex_braille_ext branch is already merged into explorer_rewrite_tmp, and so is incorporated into this new branch, so sre_latex_braille_ext probably is no longer needed. The other two look like they could be readily rebased off of explorer-rewrite-tmp. So I think it would be good to get these branched cleaned up and remove any unneeded ones.

@zorkow zorkow mentioned this pull request Dec 20, 2023
@zorkow
Copy link
Member Author

zorkow commented Dec 20, 2023

If there are other branched based off v4.0-alpha, they need to get a similar treatment. These seem to be

  • refactor/speechgenerator_pool
  • complete_explorer_functionality
  • sre_latex_braille_ext
  • explorer_rewrite_euro

The last one is just a single commit from 2cb1708, so easily recreated from the new explorer-rewrite-tmp without the v4.0-alpha dependency. The sre_latex_braille_ext branch is already merged into explorer_rewrite_tmp, and so is incorporated into this new branch, so sre_latex_braille_ext probably is no longer needed. The other two look like they could be readily rebased off of explorer-rewrite-tmp. So I think it would be good to get these branched cleaned up and remove any unneeded ones.

Is really the one refactor/speechgenerator_pool that subsumes the others and the sre_latex_braille_ext is no longer needed. I will cleanup the branches.

@zorkow
Copy link
Member Author

zorkow commented Dec 20, 2023

Closed and reopened as #1035

@zorkow zorkow closed this Dec 20, 2023
@zorkow zorkow deleted the explorer_rewrite_tmp branch January 10, 2024 14:00
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.

2 participants