1189287 - tooltip description is wrong for the camera icon.

Status: VERIFIED FIXED

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

Description

[situmam]

Attachment: mute_video.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150727205026

Steps to reproduce:

* Launch latest FF nightly
* Click on Hello button
* hover mouse over the camera icon


Actual results:

When hovering over the camera logo, a tooltip (bubble) shows up saying "mute your video".


Expected results:

This is incorrect. It should be called "turn off camera"

Comment 1

[Mark Banner (:standard8)]

Sevaan: any suggestions for wording improvements here?

"Turn off camera" isn't quite right as we don't currently turn off the camera - we just stop sending the video (the camera turn-off support would require extra work where we haven't got full platform support yet).

Comment 2

[Sevaan Franks [:sevaan]]

How about "Disable camera"?

Comment 3

[situmam]

Sounds like "Disable video" is more appropriate since you don't turn the camera off.

Comment 4

[Sevaan Franks [:sevaan]]

"Disabled video" is great.

Comment 5

[JoelBrewer01]

Hello! This is my first foray into Mozilla open source development and this looks like an easy bug for me to start out with. Looks like it hasn't been claimed yet?

Comment 6

[Mark Banner (:standard8)]

(In reply to JoelBrewer01 from comment #5)
> Hello! This is my first foray into Mozilla open source development and this
> looks like an easy bug for me to start out with. Looks like it hasn't been
> claimed yet?

Hi, its now assigned to you, so you've got it :-)

As its your first time, I'll offer a few hints straight up:

- There's more than one file in which you need to change the string.
- Additionally, you need to change the string id for our localisation folks to see the change. I think you can just add a 2 on the end of the string id.
- You'll need to update the code as well because of the string id change, in this case, our code is in the /browser/components/loop directory, and you need to look for "button_title" - you won't find the string id as a whole.

You probably will also want to check the unit tests work.

There's more information on Hello (aka Loop) development here as well:

https://wiki.mozilla.org/Loop/Development

Comment 7

[JoelBrewer01]

Attachment: bug1189287patch1 - Here is my first attempt at a fix for this bug.

My first patch to Mozilla! :)

I updated the string as well as the string id. I also updated what I believe is the relevant code in the views.js file.

Comment 8

[JoelBrewer01]

Ah. I found a bug in that review. About to upload an updated patch.

Comment 9

[JoelBrewer01]

Attachment: bug1189287patch2 - Updated patch #2.

Here is my updated patch. Realized I wasn't accounting for the different string ID for the audio button.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8648435 [details] [diff] [review]
bug1189287patch2 - Updated patch #2.

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

Joel, that's an awesome first patch - congratulations!!

There's one thing you need to change: add the new strings to the file 'browser/locales/en-US/chrome/browser/loop/loop.properties' as well, so that you can see the change in the desktop client as well.

Can you tell me how you tested the change you made? If you have any questions, please don't hesitate to ask on IRC in #loop or me directly: mikedeboer.

I'm looking forward to the next version of your patch!

Comment 11

[JoelBrewer01]

Attachment: bug1189287patch3 - Updated patch #3

Alright, here's the updated patch. I updated the second language file. I also used '===' instead of '===' in my js code.

For testing, I ran: '/browser/components/loop/run-all-loop-tests.sh' and as far as I can tell they all passed :)

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8650266 [details] [diff] [review]
bug1189287patch3 - Updated patch #3

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

I can't see anything wrong with this patch, congratulations!!

When this lands on our code repository, I'll be filing a follow-up to make the `_getTitle` function handle new string IDs better. But for now, we're good to go!

Thanks for working on this and I'm looking forward to seeing your next patch ;-)

Comment 13

[Mike de Boer [:mikedeboer]]

url:        https://hg.mozilla.org/integration/fx-team/rev/dfc8a6f5a9e2e8854317e442828d01f80add633b
changeset:  dfc8a6f5a9e2e8854317e442828d01f80add633b
user:       jahbrewski <[e-mail]>
date:       Thu Aug 20 11:33:28 2015 +0200
description:
Bug 1189287 - Update the tooltip strings for the mute and unmute video buttons inside the Hello conversation toolbar. r=mikedeboer

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Bogdan Maris, Desktop QA]

Verified that the tooltip description has changed to 'Disable video' using latest Nightly 43.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit).