Add tooltip to X icon in control center

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Site Identity and Permission Panels
P2
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Ash Grigas, Assigned: 7, Mentored)

Tracking

({access, regression})

Trunk
Firefox 51
access, regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox49 unaffected, firefox50- wontfix, firefox51 verified)

Details

(Whiteboard: [fxprivacy] [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

a year ago
Component: General → Site Identity and Permission Panels
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.
Mentor: jhofmann@mozilla.com
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [fxprivacy] → [fxprivacy] [good first bug]
(Reporter)

Updated

a year ago
Blocks: 1296707
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.
Flags: needinfo?(agrigas)
(Reporter)

Updated

a year ago
Duplicate of this bug: 1296707
Yup it's a duplicate, my bad, I didn't notice it :)
[Tracking Requested - why for this release]: see bug 1296707 comment 0
status-firefox49: --- → unaffected
status-firefox50: --- → affected
tracking-firefox50: --- → ?
Keywords: access, regression
(Assignee)

Comment 6

a year ago
Created attachment 8786508 [details] [diff] [review]
Add tooltip to X icon in control center
Assignee: nobody → mail2seven
Status: NEW → ASSIGNED
(Assignee)

Comment 7

a year ago
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
Attachment #8786508 - Flags: review?(jhofmann)
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?
Attachment #8786508 - Flags: review?(past)
Attachment #8786508 - Flags: review?(jhofmann)
Attachment #8786508 - Flags: review+
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.
Hmm, how about something like "Clear this permission setting"?
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.
Attachment #8786508 - Flags: review?(past) → review+
(Reporter)

Comment 12

a year ago
(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'
Flags: needinfo?(agrigas)
(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'?
(Reporter)

Comment 14

a year ago
(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'
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!!
(Assignee)

Comment 16

a year ago
Created attachment 8787310 [details] [diff] [review]
Add tooltip to X icon in control center
(Assignee)

Updated

a year ago
Attachment #8786508 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
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.
Attachment #8787310 - Flags: review?(jhofmann)
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!
Attachment #8787310 - Flags: review?(jhofmann) → review+
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.
Keywords: checkin-needed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dff17564e6f

Comment 21

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6cd5070f454
Add tooltip to X icon in control center. r=johannh
Keywords: checkin-needed

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6cd5070f454
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Verified fixed FX 51.0a1 (2016-09-05) Win 7, Ubuntu 14.04, OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Iteration: --- → 51.3 - Sep 12
Is this worth uplifting to 50 (my queries show it as affecting 50, too)?
Flags: needinfo?(mail2seven)
(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.
Flags: needinfo?(mail2seven)
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.
Untracked for 50, wontfix as the patch has a string change, this will ride the 51 train.
status-firefox50: affected → wontfix
tracking-firefox50: ? → -
You need to log in before you can comment on or make changes to this bug.