Closed Bug 1216791 Opened 9 years ago Closed 9 years ago

Change the tooltip of the Hello icon in the toolbar

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: sevaan, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

Attachments

(1 file, 6 obsolete files)

The tooltip of the Firefox Hello icon in the toolbar says "Start a conversation".

This should be changed immediately to reflect what Hello Firefox does now with the new journey and should include the name of the product since since that's what we refer to it as in all our marketing and users are unable to find Firefox Hello in the UI.
Sevaan, strings are agreed on bug 1211351 and will be implemented through bug 1213844.
Can this bug be closed since it's already covered in the other bugs?
Flags: needinfo?(sfranks)
I don't think this is going to be explicitly included in bug 1213844 due to it not having been specified in the original user journey story. Therefore I think we should just keep this open.
Blocks: 1213844
Flags: needinfo?(sfranks)
The icon's active state tooltip says:

"Active conversation"

https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties#99

Are we getting rid of "conversation" in preference of browsing/sharing?


In addition to the tooltip on the toolbar icon, there's also a "Start a conversation…" menu item under Tools:

https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#260

Although that might be trickier to fix than just a string change as it wouldn't always start a conversation anymore?
We should change "Active conversation" to say something like "Someone is waiting for you". I have added the string to Bug 1211351.

The "Start a conversation..." tooltip is being addressed in Bug 1216791.
There's several states for the icon:

loop-call-button3.tooltiptext = Start a conversation
loop-call-button3-error.tooltiptext = Error!
loop-call-button3-donotdisturb.tooltiptext = Do not disturb
loop-call-button3-screensharing.tooltiptext = You are sharing your screen
loop-call-button3-active.tooltiptext = Active conversation
loop-call-button3-participantswaiting.tooltiptext = Someone is waiting for you in a conversation

Although I just tested in Release and the icons do show the updated icon (filled - default, blue - in conversation, green - sharing tabs, empty - do not disturb), the tooltip always stays as "Start a conversation"...
Here is my proposition aligned with what we have on bug 1211351:
loop-call-button3.tooltiptext = Browse this page with a friend
loop-call-button3-error.tooltiptext = Error!
loop-call-button3-donotdisturb.tooltiptext = Notifications turned off
loop-call-button3-screensharing.tooltiptext = You are sharing your tabs
loop-call-button3-active.tooltiptext = You are sharing your tabs
loop-call-button3-participantswaiting.tooltiptext = Someone is waiting for you

loop-call-button3-screensharing.tooltiptext and loop-call-button3-active.tooltiptext both say "You are sharing your tabs" since when you are in a conversation in 44 you'll always be sharing your tabs.

Sevaan, sounds OK?
Flags: needinfo?(sfranks)
Sounds great. Thanks Romain!
Flags: needinfo?(sfranks)
(In reply to Romain Testard [:RT] from comment #6)
> loop-call-button3-screensharing.tooltiptext and
> loop-call-button3-active.tooltiptext both say "You are sharing your tabs"
> since when you are in a conversation in 44 you'll always be sharing your
> tabs.
What should happen when the user clicks pause or stop in the notification bar?
Pause is a new state, thanks for remembering that. We can just say "Paused" for it.

Stop doesn't need a special message since it reverts back to Browse this page with a friend.
Did we not discuss "Restart" rather than "Paused" in bug 1211351?
Sorry, you are correct. There have been so many discussions in so many different places. I felt like we'd tackled this.

"Restart"!
Whiteboard: [web sharing]
Hey Mark, I've talked offline with Romain about this string change:

loop-call-button3-donotdisturb.tooltiptext = Notifications turned off

It is for 45 so this patch does not modify it. However, we should take it into account for 45.
Attachment #8683068 - Flags: review?(standard8)
Comment on attachment 8683068 [details] [diff] [review]
Change the tooltip of the Hello icon in the toolbar

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

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +95,5 @@
>  
>  # LOCALIZATION NOTE(loop-call-button3.label): This is a brand name, request
>  # approval before you change it.
>  loop-call-button3.label = Hello
> +loop-call-button3.tooltiptext = Browse this page with a friend

As per the other bug I just commented on, we need to change the string id here. I'd suggest changing all of these to start with loop-call-button4, as this helps L10n to know they belong to the same part of the display.
Attachment #8683068 - Flags: review?(standard8) → review-
Assignee: nobody → b.mcb
Patch ready to be reviewed
Attachment #8683068 - Attachment is obsolete: true
Attachment #8683656 - Flags: review?(standard8)
Attachment #8683656 - Flags: review?(standard8) → review?(mdeboer)
Comment on attachment 8683656 [details] [diff] [review]
Change the tooltip of the Hello icon in the toolbar

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

Regrettably, this is the wrong way around! Instead of changing all the string IDs, because the code looks like it depends on it, you should change only the ID of the string that is in fact changing and update the code to not depend on the prefix anymore.
I'm pretty sure your next attempt will be spot-on ;-)
Attachment #8683656 - Flags: review?(mdeboer) → feedback+
Hey Mike, I've just following what Mark suggested in comment 13. My point is that if we change just a few ID's even if we change the code to no depend on the prefix, could be a bit confusing. But I understand that we should only change those IDs that has been changed. Let me upload a new patch
Flags: needinfo?(mdeboer)
The code has no dependency with the tooltip IDs. Could you review it?
Attachment #8686073 - Flags: review?(mdeboer)
Attachment #8683656 - Attachment is obsolete: true
(In reply to Mike de Boer [:mikedeboer] from comment #15)
> Comment on attachment 8683656 [details] [diff] [review]
> Change the tooltip of the Hello icon in the toolbar
> 
> Review of attachment 8683656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Regrettably, this is the wrong way around! Instead of changing all the
> string IDs, because the code looks like it depends on it, you should change
> only the ID of the string that is in fact changing and update the code to
> not depend on the prefix anymore.
> I'm pretty sure your next attempt will be spot-on ;-)

Mike: did you see comment 13? I thought L10n like to keep the string ids similar in this case (including the number), as it helps them identify items that are the same, even though it causes some extra string bumps. But as we're not sure, lets ask them...

Axel, which of the patches is best here? Change all the ids, or just the ones for the strings we're changing?
Flags: needinfo?(l10n)
The only real requirement is to keep together a label and its accesskey, since there are tools that display them together (or try to). There's no hard requirement for tooltips.

In this specific case I think we're changing way too many strings, forcing people to re-localize them.

Personally I would go with:

-loop-call-button3.tooltiptext = Start a conversation
+loop-call-button3.tooltiptext2 = Browse this page with a friend
-loop-call-button3-active.tooltiptext = Active conversation
+loop-call-button3-active.tooltiptext2 = You are sharing your tabs
-loop-call-button3-participantswaiting.tooltiptext = Someone is waiting for you in a conversation
+loop-call-button3-participantswaiting.tooltiptext2 = Someone is waiting for you
Flags: needinfo?(l10n)
Patch updated to fit the requirements of the l10n team.
Attachment #8687203 - Flags: review?(mdeboer)
Attachment #8686073 - Attachment is obsolete: true
Attachment #8686073 - Flags: review?(mdeboer)
Wrong patch uploaded sorry. This is the good one
Attachment #8687211 - Flags: review?(mdeboer)
Attachment #8687203 - Attachment is obsolete: true
Attachment #8687203 - Flags: review?(mdeboer)
Flags: needinfo?(mdeboer)
Comment on attachment 8687211 [details] [diff] [review]
Change the tooltip of the Hello icon in the toolbar.

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

::: browser/base/content/browser-loop.js
@@ -287,3 @@
>        } else if (aReason == "login" && this.MozLoopService.userProfile) {
>          state = "active";
> -        mozL10nId += "-active";

What if you kept all this here and simply added:

```js
suffix += "2";
```

for the strings you updated?

@@ -300,1 @@
>            }

`suffix += "2";`
Attachment #8687211 - Flags: review?(mdeboer) → feedback+
Hey Mike! I've changed the patch as you suggested in comment 22. Pleasee check it and let me know if there is something wrong =)
Attachment #8687211 - Attachment is obsolete: true
Attachment #8688860 - Flags: review?(mdeboer)
Comment on attachment 8688860 [details] [diff] [review]
Change the tooltip of the Hello icon in the toolbar.

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

LGTM! r=me with the one change below.

::: browser/base/content/browser-loop.js
@@ +304,5 @@
>            this.updateTooltiptext(mozL10nId + suffix);
>            this.toolbarButton.node.setAttribute("state", state);
>          });
>          return;
>        }

I think

```js
} else {
  suffix += "2";
}
```

would work here as well, instead of the three lines below.
Attachment #8688860 - Flags: review?(mdeboer) → review+
Last comment applied to the patch
Attachment #8688860 - Attachment is obsolete: true
Attachment #8690727 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b9a99168d92d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: