Closed
Bug 1321862
Opened 6 years ago
Closed 6 years ago
Use synth word length in Narrate word highlight
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
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 hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
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•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 6•6 years ago
|
||
(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
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/acd1e405df74
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•