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)
Firefox
Site Identity
Tracking
()
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)
1.58 KB,
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
Hover tooltip should read out: "Remove permission and ask again"
Updated•6 years ago
|
Component: General → Site Identity and Permission Panels
Comment 1•6 years ago
|
||
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]
Comment 2•6 years ago
|
||
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)
Comment 4•6 years ago
|
||
Yup it's a duplicate, my bad, I didn't notice it :)
Comment 5•6 years ago
|
||
[Tracking Requested - why for this release]: see bug 1296707 comment 0
status-firefox49:
--- → unaffected
status-firefox50:
--- → affected
tracking-firefox50:
--- → ?
Keywords: access,
regression
Updated•6 years ago
|
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 8•6 years ago
|
||
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+
Comment 9•6 years ago
|
||
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•6 years ago
|
||
Hmm, how about something like "Clear this permission setting"?
Comment 11•6 years ago
|
||
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•6 years 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)
Comment 13•6 years ago
|
||
(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•6 years 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'
Comment 15•6 years ago
|
||
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•6 years ago
|
||
Attachment #8786508 -
Attachment is obsolete: true
Assignee | ||
Comment 17•6 years 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 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
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
Comment 21•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6cd5070f454
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 23•6 years ago
|
||
Verified fixed FX 51.0a1 (2016-09-05) Win 7, Ubuntu 14.04, OS X 10.11.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Iteration: --- → 51.3 - Sep 12
Comment 24•6 years ago
|
||
Is this worth uplifting to 50 (my queries show it as affecting 50, too)?
Flags: needinfo?(mail2seven)
Comment 25•6 years ago
|
||
(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)
Comment 26•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•