Closed Bug 1321862 Opened 6 years ago Closed 6 years ago

Use synth word length in Narrate word highlight

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file)

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 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?
Attachment #8816527 - Flags: review?(gijskruitbosch+bugs)
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 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? :-)
Attachment #8816527 - Flags: review?(gijskruitbosch+bugs) → review+
(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
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/acd1e405df74
Use charLength for word highlight in Narrate. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/acd1e405df74
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.