Closed Bug 1290173 Opened 8 years ago Closed 8 years ago

Introduce word (and sentence?) tracking in Narrate

Categories

(Toolkit :: Reader Mode, defect)

Product:
Toolkit
Component:
Reader Mode
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(2 files)

Bug 1290173 - Introduce word tracking in Narrate.
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1290173 - Introduce Narrate word tracking test.
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
Narrate only highlihgts the currently spoken paragraph. It would be nice to have word and possibly sentence tracking. It would help people with disabilities, students, and users who are not 100% proficient in the given language.

It will also look really cool...
Markus, could you give me feedback on this?

Who do I ask for visual design help?
Flags: needinfo?(mjaritz)
(I just saw the try build and I'm floored - I'm so happy with this)
OMG that bouncy ball is awesome!

Having something to help keep track of the exact reading position is great. It looks like it could replace the block around a paragraph altogether if noticeable enough, but not too distracting.

My first feeling is that the underline, if it would transition from word to word, is a great option. It is recognizable on the page, but does not interrupt the line of text one follows when reading.
The word magnifier is somewhat difficult to follow for me personally, as the change in color makes the word somewhat hard to read. (And it looks to me, on black as if the highlighted word is getting bigger.)
And the bouncy ball feels somewhat distracting through its big animation when changing line, but might be good competition for the underscore.
(Do you see a value in providing different tracking options to users, or is the UI control just for testing purposes?)
What made you build these 3 tracking tools? To decide which one to use, it might be worth looking into past research to learn what supports best in reading along. Do you have material on that? (Maybe karaoke systems might be a source of data for that.)

You also mentioned 3 different groups of people who could profit from that:
* people learning a language - because they could also read the word and have a way to know which word belongs to what sound.
* students
* people with disabilities
Could you please explain why you think that students and people with disabilities would profit from that?

If we know what interaction we want for that feature, we can ask Stephen for visual design feedback and he might refer to another visual design if he has no time.
Flags: needinfo?(mjaritz) → needinfo?(eitan)
(In reply to Markus Jaritz [:maritz] (UX) from comment #5)
> OMG that bouncy ball is awesome!
> 
> Having something to help keep track of the exact reading position is great.
> It looks like it could replace the block around a paragraph altogether if
> noticeable enough, but not too distracting.

The one issue we have is that word tracking is not available on every platform and every speech engine. For example, we don't have this capability in Linux at all. In Windows, there can theoretically be 3rd part speech engines installed that don't have the capability either.

So there would need to be a graceful fallback to whatever is universally possible, in this case paragraph highlighting will always be available.

> 
> My first feeling is that the underline, if it would transition from word to
> word, is a great option. It is recognizable on the page, but does not
> interrupt the line of text one follows when reading.

Yeah, its pretty great.

> The word magnifier is somewhat difficult to follow for me personally, as the
> change in color makes the word somewhat hard to read. (And it looks to me,
> on black as if the highlighted word is getting bigger.)

It is getting bigger :) I made the effect subtle, but we can blow it up to be *much* bigger. The target audience for such a tracker is probably not you or me. It was my uneducated guess at what would be useful to users with partial vision loss or cognitive disabilities.

> And the bouncy ball feels somewhat distracting through its big animation
> when changing line, but might be good competition for the underscore.

I could fix the line change animation to be less distracting.

> (Do you see a value in providing different tracking options to users, or is
> the UI control just for testing purposes?)
> What made you build these 3 tracking tools? 

I think they may be different use cases. It doesn't mean we need to answer all of them. They are all driven by stylesheet changes, so potentially this should be very extendable via addons and custom styles.

> To decide which one to use, it
> might be worth looking into past research to learn what supports best in
> reading along. Do you have material on that? (Maybe karaoke systems might be
> a source of data for that.)

Not yet. I have been trying to find examples of reading aids that do interesting stuff for different disabilities and use cases, but I didn't have any luck yet.

> 
> You also mentioned 3 different groups of people who could profit from that:
> * people learning a language - because they could also read the word and
> have a way to know which word belongs to what sound.
> * students
> * people with disabilities
> Could you please explain why you think that students and people with
> disabilities would profit from that?
> 

I guess the students group overlaps with the other two. I am thinking about children and adults with learning disabilities, foreign language learning, and even new readers.

When I mentioned people with disabilities, it is really a wide range of use cases which includes various forms of cognitive disabilities (dyslexia and neurological impairments), and different kinds of vision loss (macular degeneration, glaucoma, etc). Each kind of vision loss is unique in how to best accommodate it.

The main feedback I am taking from your response is that the underline tracking is a good start for a general use-case. I honestly don't know if it is worth having more than one option available to the user. My original assumption was "yes", but if we should just have one style of word tracking on by default, I am open to that too.
Flags: needinfo?(eitan)
I like the idea of keeping it simple-ish for the first version.
Attachment #8794964 - Flags: review?(gijskruitbosch+bugs) → review?(mdeboer)
I've redirected to mikedeboer who (hello findbar) has some more experience than me with these types of per-word animated highlighting things.
Comment on attachment 8794964 [details]
Bug 1290173 - Introduce word tracking in Narrate.

https://reviewboard.mozilla.org/r/81158/#review79968

Cool stuff, this! I really enjoyed using it. I can see this used in several contexts, like others have mentioned in the bug.

General comments:
 - Do you plan on writing tests for this code/ feature? Do you need help with that?
 - You are so lucky you're able to control the layout of the reader mode view, because if you were to do this on a regular web page, you'd be in a world of... well, pain. FinderHighlighter.jsm is the latest example of that suffering. Just kidding, of course, but the difference in complexity shows I think.
 - Please find the other remarks inline.

Looking forward to seeing your next patch!

::: toolkit/components/narrate/Narrator.jsm:214
(Diff revision 1)
> +        if (e.name != "word") {
> +          // We are only interested in word boundaries for now.
> +          return;
> +        }
> +
> +        let rex = /\S+/g;

I think you read this as 'any non word-break character', but in practice this is not the case. It's true that for languages based on the latin alphabet this would be more-or-less sufficient, but for CJK it's another story, for example.
In other words; does the event data not contain both indices?

::: toolkit/components/narrate/Narrator.jsm:298
(Diff revision 1)
> +    let containerRect = this.container.getBoundingClientRect();
> +    let range = this._getRange(start, end);
> +    let rangeRects = range.getClientRects();
> +    let win = this.container.ownerDocument.defaultView;
> +    let computedStyle = win.getComputedStyle(range.endContainer.parentNode);
> +    let nodes = this._getHighlightNodes(rangeRects.length);

nit: can you (re-)name this a bit more specific to, for example, `getFreshHighlightNodes(count)`?

::: toolkit/components/narrate/Narrator.jsm:304
(Diff revision 1)
> +
> +    for (let i = 0; i < rangeRects.length; i++) {
> +      let r = rangeRects[i];
> +      let node = nodes[i];
> +
> +      let style = {

General comment: for all the CSS properties you might want to copy over, see https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/FinderHighlighter.jsm#646
Why do you need these styles?

Do we support `writing-mode: vertical-rl` in reader mode? If so, it'd be interesting to check out what effect it has.

Please define a common-styles block before the loop:
```js
let fontStyle = {
  "font-family": computedStyle.fontFamily,
  "font-size": computedStyle.fontSize,
  "font-style": computedStyle.fontStyle,
  "font-weight": computedStyle.fontWeight
};
```

And in the loop:
```js
let style = Object.assign({
  //...
}, fontStyle);
```

::: toolkit/components/narrate/Narrator.jsm:320
(Diff revision 1)
> +      // Enables us to vary the CSS transition on a line change.
> +      node.classList.toggle("newline", style.top != node.dataset.top);
> +      node.dataset.top = style.top;
> +
> +      // Enables CSS animations.
> +      node.classList.remove("animate");

Why does doing this enable CSS animations?

::: toolkit/components/narrate/Narrator.jsm:326
(Diff revision 1)
> +      win.requestAnimationFrame(() => {
> +        node.classList.add("animate");
> +      });
> +
> +      // Enables alternative word display with a CSS pseudo-element.
> +      node.dataset.word = range.toString();

Where are you using this? Please remove it if you're not.

::: toolkit/components/narrate/Narrator.jsm:348
(Diff revision 1)
> +
> +  _getHighlightNodes: function(count) {
> +    let doc = this.container.ownerDocument;
> +    let nodes = Array.from(this.container.querySelectorAll(
> +      ".narrate-word-highlight"));
> +    for (let i = 0; i < nodes.length - count; i++) {

nit: please explain in a comment what you're doing with the `- count` thing.
nit: please use another variable name than `i` for the second loop.

::: toolkit/components/narrate/Narrator.jsm:377
(Diff revision 1)
> +          return [node, offset - i];
> +        }
> +        i += length;
> +      } while ((node = treeWalker.nextNode()));
> +
> +      // Offset is out of bounds, return last offset of last node

nit: missing dot.

::: toolkit/themes/shared/aboutReaderControls.css:85
(Diff revision 1)
>    padding: 0;
>    list-style: none;
>    background-color: #fbfbfb;
>    -moz-user-select: none;
>    border-right: 1px solid #b5b5b5;
> +  z-index: 1;

The problem I see with fiddling with the z-indices here is that the Narrator panel, attached to the toolbar button is cut-in by the highlighted paragraphs.
Attachment #8794964 - Flags: review?(mdeboer)
Attachment #8794964 - Flags: feedback+
Comment on attachment 8794964 [details]
Bug 1290173 - Introduce word tracking in Narrate.

https://reviewboard.mozilla.org/r/81158/#review79968

> I think you read this as 'any non word-break character', but in practice this is not the case. It's true that for languages based on the latin alphabet this would be more-or-less sufficient, but for CJK it's another story, for example.
> In other words; does the event data not contain both indices?

Yeah.. this is a problem. I also tried using the `\B` word boundary character, but it is extremely limited (only Roman characters). So I had better luck with `\S`. Tested it with Hebrew and it works. I don't know anything about non-latin/semitic languages so I realize it may be limited.

The current API doesn't give both indices. I am motivated to talk to the spec group to have that added. I just looked in Windows and Mac speech synthesis, and it would be possible. Should we block on that development? I don't think it is worth waiting. Especially since synth language support is already a limited set we can test for.

> General comment: for all the CSS properties you might want to copy over, see https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/FinderHighlighter.jsm#646
> Why do you need these styles?
> 
> Do we support `writing-mode: vertical-rl` in reader mode? If so, it'd be interesting to check out what effect it has.
> 
> Please define a common-styles block before the loop:
> ```js
> let fontStyle = {
>   "font-family": computedStyle.fontFamily,
>   "font-size": computedStyle.fontSize,
>   "font-style": computedStyle.fontStyle,
>   "font-weight": computedStyle.fontWeight
> };
> ```
> 
> And in the loop:
> ```js
> let style = Object.assign({
>   //...
> }, fontStyle);
> ```

I describe below why these styles are good to have (basically for CSS extension support).

> Why does doing this enable CSS animations?

The idea here (and below), is to enable as much customizability via CSS as possible without any changes to js. This would allow extensions to restyle the word highlight very easily. So the "animate" class is removed/added here in order to initiate an animation that is more complex than a simple CSS transition. I used this in the bouncy ball example in the previous PoC build.

> Where are you using this? Please remove it if you're not.

Like above, this would enable a CSS change where you can, for example, have a larger "word bubble" highlight. Similar to the new finder highlight. This would be done with a pseudoelement like `::after` and `content: attr(data-word)`. Another usage example is to have the word appear giant in the center of the screen. This can be useful for certain visual impairmanents.

> The problem I see with fiddling with the z-indices here is that the Narrator panel, attached to the toolbar button is cut-in by the highlighted paragraphs.

This is exactly meant to remedy that. Without that rule I see the problem you describe. The toolbar and descendents need to be in a separate stacking context.
Assignee: nobody → eitan
Status: NEW → ASSIGNED
Comment on attachment 8794964 [details]
Bug 1290173 - Introduce word tracking in Narrate.

https://reviewboard.mozilla.org/r/81158/#review80238

Please look at https://www.evernote.com/l/APuEohtDknZDgKXNibWQlbncA0X1zJEH3TI which is the only visual problem I see. Other than that, this patch is almost done!
I see it's not done in this file, but it is a good practice in other Gecko JS code: documentation.
Can you add docblock-comments for the new Highlighter class and each of its methods? Code is written-once, read many times by many people.
Additionally, what do you think about adding a test? Adding one to browser_narrate.js looks like it's doable.

::: toolkit/components/narrate/Narrator.jsm:221
(Diff revision 2)
> +          return;
> +        }
> +
> +        let rex = /\S+/g;
> +        rex.lastIndex = e.charIndex;
> +        let res = rex.exec(paragraph.textContent);

I think it's worth adding your explanation as a comment here and use variable names that clarify what you're fetching here. Also make sure to note the current limitations of this approach, for future reference.
For example:
s/rex/reWordBoundary/
s/res/firstIndex/

::: toolkit/components/narrate/Narrator.jsm:307
(Diff revision 2)
> +    let computedStyle = win.getComputedStyle(range.endContainer.parentNode);
> +    let nodes = this._getFreshHighlightNodes(rangeRects.length);
> +
> +    let textStyle = {};
> +    for (let textStyleRule of kTextStylesRules) {
> +      textStyle[textStyleRule] = computedStyle[textStyleRule];

This doesn't work; `getComputedStyle()` returns CSS properties in camelCase, not hyphenated. My advise is also to just leave this code out for now and put it back in when you add a feature that needs it (future extensions that you showed in your PoC patch!)

::: toolkit/components/narrate/Narrator.jsm:320
(Diff revision 2)
> +        "top": `${r.top - containerRect.top + r.height / 2}px`,
> +        "left": `${r.left - containerRect.left + r.width / 2}px`,
> +        "width": `${r.width}px`,
> +        "height": `${r.height}px`
> +      };
> +      Object.assign(style, textStyle);

`Object.assign` returns the target object (first argument), so you can save a whole line here!

::: toolkit/components/narrate/Narrator.jsm:333
(Diff revision 2)
> +      win.requestAnimationFrame(() => {
> +        node.classList.add("animate");
> +      });
> +
> +      // Enables alternative word display with a CSS pseudo-element.
> +      node.dataset.word = range.toString();

Same advise for this code - you can always add it back when you extend this feature with more awesomeness.

::: toolkit/components/narrate/Narrator.jsm:354
(Diff revision 2)
> +  },
> +
> +  _getFreshHighlightNodes: function(count) {
> +    let doc = this.container.ownerDocument;
> +    let nodes = Array.from(this.container.querySelectorAll(
> +      ".narrate-word-highlight"));

nit: the 80-character is not a hard limit. I usually set two vertical rulers in my editor: one at 80c and another at 120c as the absolute outer limit.
Rule of thumb: if you can make it fit within 80c, please do, but if the code remains more readable when it's on the same line, don't make it longer than 120c.

::: toolkit/components/narrate/Narrator.jsm:356
(Diff revision 2)
> +  _getFreshHighlightNodes: function(count) {
> +    let doc = this.container.ownerDocument;
> +    let nodes = Array.from(this.container.querySelectorAll(
> +      ".narrate-word-highlight"));
> +
> +    // remove nodes we don't need anymore (nodes.length - count > 0).

nit: please capitalize your sentences inside comments.

::: toolkit/components/narrate/Narrator.jsm:361
(Diff revision 2)
> +    // remove nodes we don't need anymore (nodes.length - count > 0).
> +    for (let toRemove = 0; toRemove < nodes.length - count; toRemove++) {
> +      nodes.shift().remove();
> +    }
> +
> +    // add additional nodes if we need them (count - nodes.length > 0).

nit: same :)
Attachment #8794964 - Flags: review?(mdeboer) → review-
Comment on attachment 8794964 [details]
Bug 1290173 - Introduce word tracking in Narrate.

https://reviewboard.mozilla.org/r/81158/#review80238

That is a weird visual problem! Did you do `./mach build` after applying this patch? I noticed that `aboutReaderControls.css` doesn't change if you don't. And that rule change there is what fixes that issue.

I can add some jsdocs. I am not a module peer here, so I hope that it's ok with those folks..

Adding tests is not easy. We rely on the fancy fake synthesis voices which don't currently support boundary event emulation. I'll try to do something like "postMessage" to get a similar effect. I'll have that be a separate patch because it is less than trivial.

> I think it's worth adding your explanation as a comment here and use variable names that clarify what you're fetching here. Also make sure to note the current limitations of this approach, for future reference.
> For example:
> s/rex/reWordBoundary/
> s/res/firstIndex/

Sounds good!

> This doesn't work; `getComputedStyle()` returns CSS properties in camelCase, not hyphenated. My advise is also to just leave this code out for now and put it back in when you add a feature that needs it (future extensions that you showed in your PoC patch!)

Are you sure? I noticed that in the find highlighter code, and I wasn't sure why that was needed. This is true: `style.fontSize == style['font-size']`.

My plan is to blog and offer different style snippets when we ship this. Basically, we decided to keep this a simple non-configurable feature and offer configuration and options via extensions and useragent.css.

> `Object.assign` returns the target object (first argument), so you can save a whole line here!

Greatt tip!

> Same advise for this code - you can always add it back when you extend this feature with more awesomeness.

Same as above. Also, this is probably going to be useful for those tests I'm going to write :)

> nit: the 80-character is not a hard limit. I usually set two vertical rulers in my editor: one at 80c and another at 120c as the absolute outer limit.
> Rule of thumb: if you can make it fit within 80c, please do, but if the code remains more readable when it's on the same line, don't make it longer than 120c.

I have eslint set at 80 for this directory, and I would prefer to keep it. I worked around this with a small refactor by adding a getter propery for that query!
Comment on attachment 8794964 [details]
Bug 1290173 - Introduce word tracking in Narrate.

https://reviewboard.mozilla.org/r/81158/#review80912

Nice job. Ship it!

::: toolkit/components/narrate/Narrator.jsm:222
(Diff revision 3)
> +        }
> +
> +        // Match non-whitespace. This isn't perfect, but the most universal
> +        // solution for now.
> +        let reWordBoundary = /\S+/g;
> +        // Match the first word from the boundary event offset

nit: missing dot.

::: toolkit/components/narrate/Narrator.jsm:321
(Diff revision 3)
> +    let computedStyle = win.getComputedStyle(range.endContainer.parentNode);
> +    let nodes = this._getFreshHighlightNodes(rangeRects.length);
> +
> +    let textStyle = {};
> +    for (let textStyleRule of kTextStylesRules) {
> +      textStyle[textStyleRule] = computedStyle[textStyleRule];

Well, I learn everyday - evidence right here! Too bad this feature is undocumented, it seems. Thanks for explaining it to me. In time, I'll update FinderHighlighter.jsm too.

::: toolkit/components/narrate/Narrator.jsm:430
(Diff revision 3)
> +
> +  /*
> +   * Get all existing highlight nodes for container.
> +   */
> +  get _nodes() {
> +    return this.container.querySelectorAll(".narrate-word-highlight")

I see what you did here. Apart from missing a semi-colon, I'm not sure it's worth having this separately just to get this statement on a single line.
If that was your only motivation for this change, please revert it back to the way it was. My comment was merely a suggestion.

Probably unrelated, but mentioning it just in case: I know it can turn out to be a bit frustrating at times to go through more than two review cycles; I've been there as well. However, as long as your reviewers are responsive and both keep the end-goal in sight, I think that the gratification of your code shipping to millions of users is worth it!

So thanks for keeping at it!
Attachment #8794964 - Flags: review?(mdeboer) → review+
Comment on attachment 8796325 [details]
Bug 1290173 - Introduce Narrate word tracking test.

https://reviewboard.mozilla.org/r/82204/#review80916

LGTM! Please push this one to try too, for posterity, before landing.

::: toolkit/components/narrate/test/browser_word_highlight.js:38
(Diff revision 1)
> +    let details;
> +    do {
> +      promiseEvent = ContentTaskUtils.waitForEvent(content, "paragraphstart");
> +      $(NarrateTestUtils.FORWARD).click();
> +      details = (yield promiseEvent).detail;
> +    } while (details.tagName != "P");

nit: in Gecko - but prolly others too - you can use `details.localName == "p"` for normalized (in case of namespaced elements) lowercase goodness.

::: toolkit/components/narrate/test/browser_word_highlight.js:41
(Diff revision 1)
> +      $(NarrateTestUtils.FORWARD).click();
> +      details = (yield promiseEvent).detail;
> +    } while (details.tagName != "P");
> +
> +    let boundaryPat = /(\s+)\S/g;
> +    let position = { left: 0, top: 0};

nit: missing space before `}`
Attachment #8796325 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #18)
> LGTM! Please push this one to try too, for posterity, before landing.

Nevermind, I just triggered one.
Comment on attachment 8794964 [details]
Bug 1290173 - Introduce word tracking in Narrate.

https://reviewboard.mozilla.org/r/81158/#review80912

> I see what you did here. Apart from missing a semi-colon, I'm not sure it's worth having this separately just to get this statement on a single line.
> If that was your only motivation for this change, please revert it back to the way it was. My comment was merely a suggestion.
> 
> Probably unrelated, but mentioning it just in case: I know it can turn out to be a bit frustrating at times to go through more than two review cycles; I've been there as well. However, as long as your reviewers are responsive and both keep the end-goal in sight, I think that the gratification of your code shipping to millions of users is worth it!
> 
> So thanks for keeping at it!

I use that getter elsewhere as well. It's actually useful! Tried to reach you on IRC to get your approval. I'm going to keep this in. If you don't approve I'll followup this patch and remove it.
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dab634f98d57
Introduce word tracking in Narrate. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/8b2c02dcf167
Introduce Narrate word tracking test. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/dab634f98d57
https://hg.mozilla.org/mozilla-central/rev/8b2c02dcf167
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1307002
I've tested on Windows 7 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6 using Firefox 52 Beta 8 (buildID: 20170220070057) and everything looks good. 

Please see here the testing report: https://public.etherpad-mozilla.org/p/bug1290173
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Depends on: 1406360
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: