Closed Bug 1297430 Opened 6 years ago Closed 6 years ago

Add tooltip to X icon in control center

Categories

(Firefox :: Site Identity, defect, P2)

Product:
Firefox
Component:
Site Identity
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox50 - wontfix
firefox51 --- verified

People

(Reporter: agrigas, Assigned: 7, Mentored)

References

Details

(Keywords: access, regression, Whiteboard: [fxprivacy] [good first bug])

Attachments

(1 file, 1 obsolete file)

Add tooltip to X icon in control center
1.58 KB, patch
johannh
: review+
Details | Diff | Splinter Review 
Hover tooltip should read out:
"Remove permission and ask again"
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
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [fxprivacy] → [fxprivacy] [good first bug]
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)
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: nobody → mail2seven
Status: NEW → ASSIGNED
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+
(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'?
(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!!
Attachment #8786508 - Attachment is obsolete: true
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
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
https://hg.mozilla.org/mozilla-central/rev/e6cd5070f454
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.