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 and catch speech computation errors, add missing braille option #1053

Merged
merged 4 commits into from
Feb 10, 2024

Conversation

zorkow
Copy link
Member

@zorkow zorkow commented Feb 8, 2024

PR contains some corrections.

  • Builds the correct speech string from the SSML also for the mjx-container element's aria-label.
  • No speech is computed when enableEnrichment is set to false. Also catches error on speech computation when enableEnrichment is true but the element is actually not enriched. I think this currently only happens in the lab or when an enrichment error is caught.
  • Adds missing enableBraille option.

@zorkow zorkow requested a review from dpvc February 8, 2024 17:52
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.

There are a couple of items identified here for your consideration.

Also, it looks like the function

protected getSpeech(node: MmlNode): string {
const attributes = node.attributes;
if (!attributes) return '';
const speech = attributes.getExplicit('data-semantic-speech') as string;
// TODO (explorer) For tree role move all speech etc. to container
// element.
if (!attributes.hasExplicit('data-semantic-parent') && speech) {
return speech;
}
for (let child of node.childNodes) {
let value = this.getSpeech(child);
if (value) {
return value;
}
}
return '';
}

is no longer used, so can be eliminated.

Also, I'm wondering if

if (this.options.enableSpeech || this.options.enableBraille) {
...
}

should be placed around lines

for (const math of this.math) {
(math as EnrichedMathItem<N, T, D>).attachSpeech(this);
}

like is done at line 430 below. That would save having to loop through all the math when we aren't adding speech or braille.

@@ -244,12 +244,21 @@ export function EnrichedMathItemMixin<N, T, D, B extends Constructor<AbstractMat
*/
public attachSpeech(document: MathDocument<N, T, D>) {
if (this.state() >= STATE.ATTACHSPEECH) return;
if (this.isEscaped || !document.options.enableEnrichment) {
this.state(STATE.ATTACHSPEECH);
Copy link
Member

Choose a reason for hiding this comment

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

Do any of the calls below cause a MathJax retry error? (That is, do any call mathjax.retryAfter()?)

If not, then you can do this.state(STATE.ATTACHSPEECH) above line 247, and then just return here and the other places where the state is set. That would avoid having to set the state in several places. But it only works if the rest of the code will always complete and not restart the typesetting via retryAfter().

Copy link
Member Author

Choose a reason for hiding this comment

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

It should not. The only place that triggers a retryAfter is in line 183:

this.generatorPool.init(document.options, document.adaptor);

But that should have run before attachSpeech at all times.

Comment on lines 253 to 254
if (!speech || !braille ||
document.options.enableSpeech || document.options.enableBraille) {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch this earlier, but I think this condition is wrong. That is, if document.options.enableSpeech is false, you should not generate speech even if !speech is true, and similarly for braille. So I think this should be

if ((!speech && document.options.enableSpeech) || (!bralle && document.options.enableBraille)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Good catch.

Base automatically changed from refactor/explorer_depth to develop February 9, 2024 08:28
@zorkow
Copy link
Member Author

zorkow commented Feb 9, 2024

I've made changes except the attachSpeech one and I've made a change in package.json to the new SRE release.

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.

OK, this all looks good. If you want to do the STATE.ATTACHSPEECH change, that is great. Approved either way.

@zorkow
Copy link
Member Author

zorkow commented Feb 10, 2024

OK, this all looks good. If you want to do the STATE.ATTACHSPEECH change, that is great. Approved either way.

I've tested with the states and I've made the simplification.

@zorkow zorkow merged commit 611d09f into develop Feb 10, 2024
@zorkow zorkow deleted the fix/enrichment_switch branch February 10, 2024 22:22
@dpvc dpvc added this to the v4.0 milestone Apr 17, 2024
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.

None yet

2 participants