1131542 - Loop button on toolbar needs different tooltips to explain colours/state

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

Split out from bug 1126334. We really need a some tooltips to explain the different states of the Hello button on the toolbar. Especially for screen sharing, as there's no other indication in the panel as to why.

Currently, we have the tooltip as "Start a conversation".

Sevaan, can you come up with some suggestions for tooltips here?

Marking as a P1 as I think this is an important part of the screensharing work

Comment 1

[Sevaan Franks [:sevaan]]

Some initial thoughts..

Left-to-Right, based on this image: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/loop/[e-mail]

- Start a Conversation
- Do Not Disturb
- Error!
- You are sharing your screen

For the blue icon:

If user does not have conversation window open and someone is in one of their rooms...
- Someone is waiting for you in a Conversation!

If user has an open conversation...
- Active Conversation

Active Conversation doesn't feel right, but I'm drawing a bit of a blank on how better to communicate it. 

Matej, do you mind chiming in on the Tooltips above?

Comment 2

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #1)

> If user does not have conversation window open and someone is in one of
> their rooms...
> - Someone is waiting for you in a Conversation!

Maybe just "Someone is waiting for you" could work?

> If user has an open conversation...
> - Active Conversation

This could be "Current Conversation" or "Open Conversation"

Comment 3

[Sevaan Franks [:sevaan]]

(In reply to Matej Novak [:matej] from comment #2)
> This could be "Current Conversation" or "Open Conversation"

Good suggestions, Matej. Thanks. I like "Open Conversation".

:standard8 - Can we have it say "Open Conversations" if there is more than one active room?

Comment 4

[Mark Banner (:standard8)]

(In reply to Sevaan Franks [:sevaan] from comment #3)
> :standard8 - Can we have it say "Open Conversations" if there is more than
> one active room?

Yes we can. Although I'd point out that "Open Conversation" sounds to me a little like an action the user can do (because of the tooltip context), rather than the current state.

Comment 5

[Sevaan Franks [:sevaan]]

 
> Yes we can. Although I'd point out that "Open Conversation" sounds to me a
> little like an action the user can do (because of the tooltip context),
> rather than the current state.

That's true. Matej, was there a reason Active Conversation/Active Conversations didn't appeal to you?

Comment 6

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #5)
>  
> > Yes we can. Although I'd point out that "Open Conversation" sounds to me a
> > little like an action the user can do (because of the tooltip context),
> > rather than the current state.
> 
> That's true. Matej, was there a reason Active Conversation/Active
> Conversations didn't appeal to you?

It just didn't feel as natural to me in terms of the way a person might think about it or say it, but give the potential ambiguity, I'm OK using it.

Comment 7

[Sevaan Franks [:sevaan]]

Sounds good. Let's do it.

Comment 8

[:shell escalante]

updated user story from Sevaan & Matej's comments.  Are we good to turn this into to an implementation bug?

Mark is there any other scenario you needed language for - or does the User Story cover it all?

The only language that I didn't see 100% resolved was between "Someone is waiting for you in a Conversation!" and "Someone is waiting for you".  I put a compromise: "Someone is waiting for you in a Conversation" (no exclamation point - as marketing was always anti-exclamation points, but left "in a Conversation" since Hello is new enough they might not know where).  Please Sevaan or Matej update the User Story if you prefer it otherwise.

Comment 9

[Matej Novak [:matej]]

(In reply to sescalante from comment #8)
> updated user story from Sevaan & Matej's comments.  Are we good to turn this
> into to an implementation bug?
> 
> Mark is there any other scenario you needed language for - or does the User
> Story cover it all?
> 
> The only language that I didn't see 100% resolved was between "Someone is
> waiting for you in a Conversation!" and "Someone is waiting for you".  I put
> a compromise: "Someone is waiting for you in a Conversation" (no exclamation
> point - as marketing was always anti-exclamation points, but left "in a
> Conversation" since Hello is new enough they might not know where).  Please
> Sevaan or Matej update the User Story if you prefer it otherwise.

I'm good with that, but "conversation" should be lowercase. Thanks.

Comment 10

[Mark Banner (:standard8)]

I think we're good, we just need to implement it now.

Comment 11

[:shell escalante]

Thanks Matej & Mark - will go ahead an put in the implementation backlog.

Comment 13

[Manuel Casas Barrado [:mancas]]

Attachment: Fix: Loop button on toolbar has different tooltips to explain colours/state

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8670766 [details] [diff] [review]
Fix: Loop button on toolbar has different tooltips to explain colours/state

Review of attachment 8670766 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't currently pass tests - if you're using run-all-loop-tests.sh, then unfortunately its not picking up all errors - see bug 1213213.

You can see the errors by running:

./mach mochitest browser/components/loop/test/mochitest

or

./mach mochitest browser/components/loop/test/mochitest/browser_toolbarbutton.js

Please can you also add tests for checking the tooltips are set correctly.

::: browser/base/content/browser-loop.js
@@ +268,3 @@
>        } else if (this.MozLoopService.screenShareActive) {
>          state = "action";
> +        mozL10nId += "screensharing";

I think this wants a dash at the start. You also need to add the string to the properties file.

@@ +284,5 @@
> +          }
> +
> +          this.updateTooltiptext(mozL10nId + suffix);
> +        });
> +        return;

Unfortunately this return means the toolbar button doesn't get the state set and no longer changes colour.

Comment 15

[Manuel Casas Barrado [:mancas]]

Attachment: Loop button on toolbar needs different tooltips to explain colours/state

Minor issues fixed and more tests have been added to check if the tooltiptext updates accordingly with the status

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8673614 [details] [diff] [review]
Loop button on toolbar needs different tooltips to explain colours/state

Review of attachment 8673614 [details] [diff] [review]:
-----------------------------------------------------------------

I think we're almost there now. run-all-loop-tests.sh is currently failing for two things:

- eslint: "executeSoon" needs adding to the list of globals in browser/components/loop/test/mochitest/.eslintrc
- There's another test failure in mochitests, similar to what we've already hit:

262 INFO TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_loop.js | A promise chain failed to handle a rejection:  - at chrome://browser/content/browser.js:4814 - TypeError: rooms is undefined
Stack trace:
LoopUI.roomsWithNonOwners/</<@chrome://browser/content/browser.js:4814:1
LoopRoomsInternal.getAll/<@resource:///modules/loop/LoopRooms.jsm:590:7
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:21
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:451:5
Promise.prototype.catch@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:470:10
LoopRoomsInternal.getAll@resource:///modules/loop/LoopRooms.jsm:544:1
this.LoopRooms.getAll@resource:///modules/loop/LoopRooms.jsm:1023:12
LoopUI.roomsWithNonOwners/<@chrome://browser/content/browser.js:4812:9
Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:385:5
LoopUI.roomsWithNonOwners@chrome://browser/content/browser.js:4811:1

I am thinking it might be worth modifying the LoopUI.roomsWithNonOwners function to cope with a null rooms object, but maybe try and see what's happening in the test first?

Comment 17

[Manuel Casas Barrado [:mancas]]

Attachment: Loop button on toolbar needs different tooltips to explain colours/state

I've fixed all the tests Mark. But wdyt if we modify the LoopUI.roomsWithNonOwners function to cope with null objects in order to avoid future problems?

Thanks!

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8674768 [details] [diff] [review]
Loop button on toolbar needs different tooltips to explain colours/state

Review of attachment 8674768 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this looks good. I can't see that other rooms error not normally happening, if it does then having an error will mean its more likely that someone spots it and figures out why.

r=Standard8

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e3d9d9c37584

Comment 20

[Phil Ringnalda (:philor)]

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

Comment 21

[Bogdan Maris, Desktop Test Engineering]

Verified using latest Nightly 44.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit), that the tooltips for all Hello icon states are correctly displayed just as described in User Story.