1321862 - Use synth word length in Narrate word highlight

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Eitan Isaacson [:eeejay]]

We added a new attribute to the speech synthesis boundary events: charLength.

It will be part of the spec soon. It allows us to know the length of the word as the speech engine defines it.

This will allow us to remove any shaky regex we have in place for word boundary detection in Narrate's word highlight.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1321862 - Use charLength for word highlight in Narrate.

Review commit: https://reviewboard.mozilla.org/r/97240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97240/

Comment 2

[:Gijs (back Fri Oct 7; he/him)]

Comment on attachment 8816527 [details]
Bug 1321862 - Use charLength for word highlight in Narrate.

https://reviewboard.mozilla.org/r/97240/#review97586

::: toolkit/components/narrate/Narrator.jsm:230
(Diff revision 1)
>          let reWordBoundary = /\S+/g;
>          // Match the first word from the boundary event offset.
>          reWordBoundary.lastIndex = e.charIndex;
>          let firstIndex = reWordBoundary.exec(paragraph.textContent);
> -        if (firstIndex) {
> -          highlighter.highlight(firstIndex.index, reWordBoundary.lastIndex);
> +        if (firstIndex && e.charLength) {
> +          highlighter.highlight(firstIndex.index, e.charLength);

Shouldn't we also send this in the test event, and then get rid of the last index regexy stuff completely?

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8816527 [details]
Bug 1321862 - Use charLength for word highlight in Narrate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97240/diff/1-2/

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 8816527 [details]
Bug 1321862 - Use charLength for word highlight in Narrate.

https://reviewboard.mozilla.org/r/97240/#review97586

> Shouldn't we also send this in the test event, and then get rid of the last index regexy stuff completely?

Yeah, I broke the tests. oops. The reason I kept it is because the spec says that a word boundary event's charIndex can be at the end of the last word or the beginning of the current (ie. no promise that the range does not contain whitespace). So I figured a regex is worth keeping for stripping the whitespace.

On second thought, we are the implementors of our speech API, and we already make sure not to include whitespace then we are good. So I removed the regex.

Comment 5

[:Gijs (back Fri Oct 7; he/him)]

Comment on attachment 8816527 [details]
Bug 1321862 - Use charLength for word highlight in Narrate.

https://reviewboard.mozilla.org/r/97240/#review100306

::: toolkit/components/narrate/Narrator.jsm:224
(Diff revision 2)
>            // We are only interested in word boundaries for now.
>            return;
>          }
>  
> -        // Match non-whitespace. This isn't perfect, but the most universal
> -        // solution for now.
> +        if (e.charLength) {
> +          highlighter.highlight(e.charIndex, e.charLength);

So, I hope that the units that the API uses for word length are guaranteed to be the same, considering the mess that is unicode text? :-)

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8816527 [details]
> Bug 1321862 - Use charLength for word highlight in Narrate.
> 
> https://reviewboard.mozilla.org/r/97240/#review100306
> 
> ::: toolkit/components/narrate/Narrator.jsm:224
> (Diff revision 2)
> >            // We are only interested in word boundaries for now.
> >            return;
> >          }
> >  
> > -        // Match non-whitespace. This isn't perfect, but the most universal
> > -        // solution for now.
> > +        if (e.charLength) {
> > +          highlighter.highlight(e.charIndex, e.charLength);
> 
> So, I hope that the units that the API uses for word length are guaranteed
> to be the same, considering the mess that is unicode text? :-)

From an initial test with Hebrew on Mac. Yes, it works OK :P

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acd1e405df74
Use charLength for word highlight in Narrate. r=Gijs

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/acd1e405df74