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

VERIFIED FIXED in Firefox 44

Status

P2
normal
Rank:
27
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: standard8, Assigned: mancas)

Tracking

unspecified
mozilla44
Points:
5
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox44 verified)

Details

(Whiteboard: [improve UX])

User Story

Split out from bug 1126334. We need 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.

Sevaan's initial thoughts..

Left-to-Right, based on this image: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/loop/toolbar@2x.png

- 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 Conversations (if more than one)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
Flags: needinfo?(sfranks)
(Reporter)

Updated

4 years ago
Blocks: 1100391
Some initial thoughts..

Left-to-Right, based on this image: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/loop/toolbar@2x.png

- 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?
Flags: needinfo?(sfranks) → needinfo?(matej)

Updated

4 years ago
Whiteboard: [UX]

Comment 2

4 years ago
(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"
Flags: needinfo?(matej)
(Reporter)

Updated

4 years ago
Blocks: 1099241
No longer blocks: 1100391
(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?
Flags: needinfo?(standard8)

Updated

4 years ago
backlog: Fx38+ → backlog+
Rank: 5
Flags: firefox-backlog+
(Reporter)

Comment 4

4 years ago
(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.
Flags: needinfo?(standard8)
 
> 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?
Flags: needinfo?(matej)

Comment 6

4 years ago
(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.
Flags: needinfo?(matej)
Sounds good. Let's do it.

Updated

4 years ago
User Story: (updated)

Comment 8

4 years ago
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.
Flags: needinfo?(standard8)
Flags: needinfo?(sfranks)
Flags: needinfo?(matej)

Comment 9

4 years ago
(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.
Flags: needinfo?(matej)
(Reporter)

Comment 10

4 years ago
I think we're good, we just need to implement it now.
Flags: needinfo?(standard8)

Comment 11

4 years ago
Thanks Matej & Mark - will go ahead an put in the implementation backlog.
Rank: 5 → 7
User Story: (updated)
Flags: needinfo?(sfranks)
Whiteboard: [UX] → [UX bug][screensharing]

Updated

4 years ago
Assignee: nobody → mdeboer
Flags: qe-verify+
Whiteboard: [UX bug][screensharing] → [screensharing]
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1084250

Updated

4 years ago
Assignee: mdeboer → nobody

Updated

4 years ago
Rank: 7 → 29
Flags: needinfo?(sescalante)
Priority: P1 → P2

Updated

3 years ago
Rank: 29 → 24
Flags: needinfo?(sescalante)

Updated

3 years ago
Rank: 24 → 27

Updated

3 years ago
Whiteboard: [screensharing] → [improve UX]

Updated

3 years ago
Points: --- → 2
(Assignee)

Updated

3 years ago
Assignee: nobody → b.mcb
(Assignee)

Comment 13

3 years ago
Created attachment 8670766 [details] [diff] [review]
Fix: Loop button on toolbar has different tooltips to explain colours/state
Attachment #8670766 - Flags: review?(standard8)
(Reporter)

Comment 14

3 years ago
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.
Attachment #8670766 - Flags: review?(standard8) → review-
(Assignee)

Comment 15

3 years ago
Created attachment 8673614 [details] [diff] [review]
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
Attachment #8670766 - Attachment is obsolete: true
Attachment #8673614 - Flags: review?(standard8)
(Reporter)

Comment 16

3 years ago
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?
Attachment #8673614 - Flags: review?(standard8) → review-
(Assignee)

Comment 17

3 years ago
Created attachment 8674768 [details] [diff] [review]
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!
Attachment #8673614 - Attachment is obsolete: true
Attachment #8674768 - Flags: review?(standard8)
(Reporter)

Comment 18

3 years ago
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
Attachment #8674768 - Flags: review?(standard8) → review+
(Reporter)

Updated

3 years ago
Iteration: --- → 44.2 - Oct 19
Points: 2 → 5
https://hg.mozilla.org/mozilla-central/rev/e3d9d9c37584
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.