1257953 - Modelessify the narrate popup

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[(no longer active)]

Right now dismissing the narrate popup stops narration.  This is extremely annoying as it often times overlays the content being read out lot, making it impossible to both see and listen to the content at the same time.

We should try to make this popup modeless, I think.

Comment 1

[Eitan Isaacson [:eeejay]]

The reason it is that way now is because the user would have no way of knowing what is speaking and why when the dialog goes away. I think we need to have an "active" version of the narrate icon to show the user that it is being used.

Phillipp, perhaps someone on your team could help with this? Give input?

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yeah, an active state for the icon should do the trick here.
Stephen, could you provide one?

Comment 3

[Tim Nguyen :ntim]

(In reply to Out of office until March 28! Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #2)
> Yeah, an active state for the icon should do the trick here.
> Stephen, could you provide one?

It would be nice if the active state for the icon were actually animated.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #3)
> (In reply to Out of office until March 28! Philipp Sackl [:phlsa] (Firefox
> UX) please use needinfo from comment #2)
> > Yeah, an active state for the icon should do the trick here.
> > Stephen, could you provide one?
> 
> It would be nice if the active state for the icon were actually animated.

Yes! I wanted to make throbby bars, but I'll leave it to someone with actual talent to design.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: reader-mode-icons-update.patch

This patch adds an active animated state for the narrate icon. Also updates the other icons to be more inline with the rest of our iconography.

What it looks like: http://cl.ly/1Z451z2J2O0g

Animated narrate icon: http://people.mozilla.org/~shorlander/narration-animation/narrate-active.svg

Comment 6

[Eitan Isaacson [:eeejay]]

Markus, as the person who gave the original UX feedback on the narrate feature. Can you try this build and see that it works the way it should?

I made it that scrolling doesn't hide the dialog, but clicking anywhere does. The speech continues after the dialog goes away.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e2756591dfa

Comment 7

[Markus Jaritz [:designakt] (UX)]

Hi Eitan, 
being able to hide the dialog feels like a great improvement to me. And I love that the icon indicate that it is still active with such a nice animation. :-) I love using the feature, and the hiding made it even better.

(One detail crossed my mind while using it with the dialog hidden. I somehow expected to get back to stopping the playback quicker after the dialog is hidden. I wonder if the dialog should re-open on hover while active... No strong feeling on that one though, and it's hard to tell if that would actually improve something.)

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to Markus Jaritz [:maritz] (UX) from comment #7)
> Hi Eitan, 
> being able to hide the dialog feels like a great improvement to me. And I
> love that the icon indicate that it is still active with such a nice
> animation. :-) I love using the feature, and the hiding made it even better.
> 

Great!

> (One detail crossed my mind while using it with the dialog hidden. I somehow
> expected to get back to stopping the playback quicker after the dialog is
> hidden. I wonder if the dialog should re-open on hover while active... No
> strong feeling on that one though, and it's hard to tell if that would
> actually improve something.)

Mouse hover interactions are tricky for accessibility.

I think it would be cool to have a minimized control (maybe even just a stop button) that appears when speaking and the dialog is dismissed. It would take 0 to little horizontal space, so it wouldn't obscure the article.

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1257953 - Update Reader Mode Icons and Add an Active State for Narrate. r=Gijs

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

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1257953 - Update narrate to use new animated icon when speaking. r=Gijs

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

Comment 11

[:Gijs (he/him)]

Comment on attachment 8744443 [details]
MozReview Request: Bug 1257953 - Update Reader Mode Icons and Add an Active State for Narrate. r=Gijs

https://reviewboard.mozilla.org/r/48545/#review45463

::: toolkit/themes/shared/narrate/narrate-active.svg:7
(Diff revision 1)
> +    #glyph-waveform {
> +      fill: #58bf43;
> +    }

This can be an attribute as well.

::: toolkit/themes/shared/narrate/narrate.svg:8
(Diff revision 1)
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24">
> +  <style>
> +    #glyph-waveform {
> +      fill: #808080;

You can just use the fill attribute and omit the `<style>` element.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8744444 [details]
MozReview Request: Bug 1257953 - Update narrate to use new animated icon when speaking. r=Gijs

https://reviewboard.mozilla.org/r/48547/#review45467

r=me with the test comment addressed.

::: toolkit/components/narrate/test/browser_narrate.js:99
(Diff revision 1)
>  
> -    promiseEvent = ContentTaskUtils.waitForEvent(content, "paragraphend");
>      toggle.click();
> -    yield promiseEvent;
>      ok(!NarrateTestUtils.isVisible(popup), "popup is dismissed while speaking");
> -    ok(true, "speech stopped when popup is dismissed");
> +    ok(true, "speech still continues after popup is dismissed");

This used to test something because of the yield for paragraphend, but now it's just a dead ok(true) - can we actually test that speech is still ongoing?

We should also make sure that the end of the test stops any other speech, it feels like.

Comment 13

[Eitan Isaacson [:eeejay]]

Comment on attachment 8744443 [details]
MozReview Request: Bug 1257953 - Update Reader Mode Icons and Add an Active State for Narrate. r=Gijs

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

Comment 14

[Eitan Isaacson [:eeejay]]

Comment on attachment 8744444 [details]
MozReview Request: Bug 1257953 - Update narrate to use new animated icon when speaking. r=Gijs

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd649eca8ea9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fc5be17759a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd649eca8ea9
https://hg.mozilla.org/mozilla-central/rev/4fc5be17759a

Comment 17

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OSX 10.10.5 using latest Aurora 49.0a2, buildID: 20160623004024.