1297430 - Add tooltip to X icon in control center

Status: VERIFIED FIXED

Description

[Ash Grigas]

Hover tooltip should read out:
"Remove permission and ask again"

Comment 1

[Johann Hofmann [:johannh]]

To fix this bug you need to add an attribute "tooltiptext" with above text to the (x) button that is used to remove web permissions. (You can find it by going to e.g. https://permission.site, "Always Allow" some permission and clicking the (i) icon in the upper left corner of your browser).

A good place to add the attribute would be here, right below the "class" attribute: http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/browser/base/content/browser.js#7361

You can't simply assign the attribute the raw string, because we need to put it in a translation file for translations. The file you should put the actual text in is http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties, you could maybe name the key "permissions.remove.tooltip".

To see how to retrieve this for usage in your code check out http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/browser/base/content/browser.js#3021

If you're completely new to contributing to Firefox check out https://developer.mozilla.org/en-US/docs/Tools/Contributing to get started.

Comment 2

[Dão Gottwald [::dao]]

How is this different from bug 1296707? Is it just a duplicate? If so we should mark it as such instead of tracking the same thing in two bugs.

Comment 4

[Johann Hofmann [:johannh]]

Yup it's a duplicate, my bad, I didn't notice it :)

Comment 5

[Dão Gottwald [::dao]]

[Tracking Requested - why for this release]: see bug 1296707 comment 0

Comment 6

[[:7]]

Created attachment 8786508 [details] [diff] [review]
Add tooltip to X icon in control center

Comment 7

[[:7]]

Comment on attachment 8786508 [details] [diff] [review]
Add tooltip to X icon in control center

Hi Johann,

I followed your suggestions and would be happy if you could review the patch.

Best regards

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8786508 [details] [diff] [review]
Add tooltip to X icon in control center

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

Hey 7, thanks for contributing. Your patch looks great to me.

Since I'm not a browser peer we need someone else to give this a final look. How about Panos?

Comment 9

[Dão Gottwald [::dao]]

I have concerns about "Remove permission and ask again":

1. I think "ask again" might raise more questions than it clarifies. E.g. when will Firefox ask again, to what end...?

2. "Remove permission" feels a bit weird when the permission was denied, i.e. the state is "Block" and there's no permission to remove.

Comment 10

[Johann Hofmann [:johannh]]

Hmm, how about something like "Clear this permission setting"?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8786508 [details] [diff] [review]
Add tooltip to X icon in control center

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

I concur with Johann :)

This is ready to land once we have the final wording for the string.

Comment 12

[Ash Grigas]

(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 8786508 [details] [diff] [review]
> Add tooltip to X icon in control center
> 
> Review of attachment 8786508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I concur with Johann :)
> 
> This is ready to land once we have the final wording for the string.

I think 'ask again' is important because users need to know how they'll set it again. I'm okay with 'Clear this permission and ask again'

Comment 13

[Johann Hofmann [:johannh]]

(In reply to agrigas from comment #12)
> (In reply to Panos Astithas [:past] from comment #11)
> > Comment on attachment 8786508 [details] [diff] [review]
> > Add tooltip to X icon in control center
> > 
> > Review of attachment 8786508 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I concur with Johann :)
> > 
> > This is ready to land once we have the final wording for the string.
> 
> I think 'ask again' is important because users need to know how they'll set
> it again. I'm okay with 'Clear this permission and ask again'

Maybe we should be a bit more precise on when Firefox will ask again. How about 'Clear this permission and ask again in the future'?

Comment 14

[Ash Grigas]

(In reply to Johann Hofmann [:johannh] from comment #13)
> (In reply to agrigas from comment #12)
> > (In reply to Panos Astithas [:past] from comment #11)
> > > Comment on attachment 8786508 [details] [diff] [review]
> > > Add tooltip to X icon in control center
> > > 
> > > Review of attachment 8786508 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I concur with Johann :)
> > > 
> > > This is ready to land once we have the final wording for the string.
> > 
> > I think 'ask again' is important because users need to know how they'll set
> > it again. I'm okay with 'Clear this permission and ask again'
> 
> Maybe we should be a bit more precise on when Firefox will ask again. How
> about 'Clear this permission and ask again in the future'?

unless we can say specifically when which i dont think we can i think its fine as is since we also have the text that says at the bottom of the panel 'you may need to reload this page for changes to take affect'

Comment 15

[Johann Hofmann [:johannh]]

7, can you please change the text to say "Clear this permission and ask again"? Apart from that, this should be ready to land.

Thanks!!

Comment 16

[[:7]]

Created attachment 8787310 [details] [diff] [review]
Add tooltip to X icon in control center

Comment 17

[[:7]]

Comment on attachment 8787310 [details] [diff] [review]
Add tooltip to X icon in control center

Hi Johann,

Thank you very much for your quick input. I changed the String to the agreed content.

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8787310 [details] [diff] [review]
Add tooltip to X icon in control center

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

Thanks for the quick update!

Comment 19

[Johann Hofmann [:johannh]]

Great! Now this needs a run on our try server, to check if tests are still green. (https://wiki.mozilla.org/ReleaseEngineering/TryServer). I just did that for you, but in the future you might want to get your own try access. I've also set the checkin-needed flag. This flag signals that we want the patch merged. If the try looks good, someone will come around and check it in for us.

Comment 20

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dff17564e6f

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6cd5070f454
Add tooltip to X icon in control center. r=johannh

Comment 22

[Wes Kocher (:KWierso)]

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

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 51.0a1 (2016-09-05) Win 7, Ubuntu 14.04, OS X 10.11.

Comment 24

[Andrew Overholt [:overholt]]

Is this worth uplifting to 50 (my queries show it as affecting 50, too)?

Comment 25

[Johann Hofmann [:johannh]]

(In reply to Andrew Overholt [:overholt] from comment #24)
> Is this worth uplifting to 50 (my queries show it as affecting 50, too)?

Yup, thanks for catching that. I didn't see Dao requested tracking but I agree that this improves accessibility and is very unlikely to cause any troubles, so we should uplift.

Comment 26

[Johann Hofmann [:johannh]]

Correction, I just remembered that we're changing a string in this patch. So unfortunately we can't uplift this to 50. Sorry for the confusion.

Comment 27

[Ritu Kothari (:ritu)]

Untracked for 50, wontfix as the patch has a string change, this will ride the 51 train.