Closed
Bug 1206246
Opened 9 years ago
Closed 8 years ago
Show indication when some permissions have been granted to the current site
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
People
(Reporter: MarcoM, Assigned: johannh)
References
Details
(Whiteboard: [fxprivacy])
Attachments
(2 files, 1 obsolete file)
No description provided.
Flags: qe-verify?
Reporter | ||
Updated•9 years ago
|
Priority: P3 → P2
Reporter | ||
Updated•9 years ago
|
Priority: P2 → P3
Reporter | ||
Updated•9 years ago
|
Priority: P3 → P1
Updated•9 years ago
|
Flags: qe-verify? → qe-verify+
Reporter | ||
Updated•9 years ago
|
QA Contact: paul.silaghi
Updated•9 years ago
|
Priority: P1 → P2
Comment 1•8 years ago
|
||
The latest mockup no longer uses a "dot", but instead replaces the "i" icon with a red icon representing what's actually being shared: https://mozilla.invisionapp.com/share/AF71R266U#/screens/143001431
Depends on: 1206245
Summary: Show the "dot" state in the "i" icon when necessary → Animate icons of the permission being used instead of the "i" icon when necessary
Comment 2•8 years ago
|
||
This bug refers to the gray dot rather than the colored one.
Aislinn, is it correct that we should display the gray dot on the "i" icon when there are customized permissions in the Control Center? Does this case happen if and only if the "permissions" section is not empty?
Flags: needinfo?(agrigas)
Summary: Animate icons of the permission being used instead of the "i" icon when necessary → Show indication when some permissions have been granted to the current site
Comment 3•8 years ago
|
||
Or are we ignoring blocked permissions for the purpose of the "dot" state of the icon?
Comment 4•8 years ago
|
||
If a user has granted a permission through the notification prompt the gray dot appears except for if it is a live streaming permission like video or microphone in which case we have a different treatment.
If a permission is not granted the icon with the line through it shows instead
Of using the gray dot on the "I".
Flags: needinfo?(agrigas)
Comment 5•8 years ago
|
||
If a user has granted a permission through the notification prompt the gray dot appears except for if it is a live streaming permission like video or microphone in which case we have a different treatment.
If a permission is not granted the icon with the line through it shows instead
Of using the gray dot on the "I".
Updated•8 years ago
|
Assignee: nobody → jhofmann
Iteration: --- → 49.3 - Jun 6
Priority: P2 → P1
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
Aislinn, do you think it's possible to add the dot icon to this existing SVG icon set for identity icons?
https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-icon.svg
If not, I would need to know where I can find a file for the dot icon that looks like the above identity icons in color and size.
Flags: needinfo?(agrigas)
Comment 7•8 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #6)
> Aislinn, do you think it's possible to add the dot icon to this existing SVG
> icon set for identity icons?
>
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> identity-block/identity-icon.svg
>
> If not, I would need to know where I can find a file for the dot icon that
> looks like the above identity icons in color and size.
the new dropbox link was posted in irc and is here:
https://www.dropbox.com/sh/rffviyl7m4x7p7c/AABoqFM6EqO3opLSds01TnYla?dl=0
Flags: needinfo?(agrigas)
Comment 8•8 years ago
|
||
Johann, you may have to add sprites manually to the combined SVG we already have in the tree - it shouldn't be too bad if we have the reference glyphs.
On the other hand it's unclear to me what the hover state for the "i" icon should be.
Bryan, should the dot be colored in blue as well? The dot disappears in the current UI mockups, but I guess it's not the intended behavior.
Flags: needinfo?(bbell)
Reporter | ||
Updated•8 years ago
|
Iteration: 49.3 - Jun 6 → 50.1
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58488/
Attachment #8761181 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 10•8 years ago
|
||
This patch adds the icons that Bryan provided. I attached a short recording of the new look and feel. I raised some concerns in yesterday's meeting because the (i) is much less prominent now. Not sure how to proceed from here.
Assignee | ||
Comment 11•8 years ago
|
||
See https://bugzilla.mozilla.org/show_bug.cgi?id=1278926#c4 for what I think is consensus about the identity block design now. I'll update the patch accordingly.
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8761181 [details]
Bug 1206246 - Change identity icon for sites with granted permissions.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58488/diff/1-2/
Assignee | ||
Comment 13•8 years ago
|
||
Here's another GIF
Attachment #8761194 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8761181 -
Flags: review?(paolo.mozmail) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8761181 [details]
Bug 1206246 - Change identity icon for sites with granted permissions.
https://reviewboard.mozilla.org/r/58488/#review55824
Looks great! r+ with the following changes.
::: browser/themes/shared/identity-block/identity-block.inc.css
(Diff revision 2)
> -#identity-box:hover > :not(#identity-icon),
> -#identity-box[open=true] > :not(#identity-icon) {
> - filter: grayscale(100%);
> -}
> -
Remove this part from this patch, file a new bug with just this change indicating it backs out bug 1223049, carry over r+, land on mozilla-central, and nominate the patch on the other bug for uplift to Beta.
::: browser/themes/shared/identity-block/identity-block.inc.css:99
(Diff revision 2)
> +#identity-box.grantedPermissions > #identity-icon {
> + list-style-image: url(chrome://browser/skin/identity-icon.svg#notice);
> +}
> +
> +#identity-box.grantedPermissions:hover > #identity-icon,
> +#identity-box.grantedPermissions:active > #identity-icon,
> +#identity-box.grantedPermissions[open=true] > #identity-icon {
> + list-style-image: url(chrome://browser/skin/identity-icon.svg#notice-hover);
> +}
> +
> #identity-box:hover > #identity-icon,
> +#identity-box:active > #identity-icon,
> #identity-box[open=true] > #identity-icon {
> list-style-image: url(chrome://browser/skin/identity-icon.svg#hover);
> }
Is it possible to have the ":active" state without the ":hover" state? In other words, I wonder if the new selector is fixing an existing issue with the hover state. If not, you can just remove it for simplicity.
nit: Order the rules as either "#normal, #hover, #notice, #notice-hover" or "#normal, #notice, #hover, #notice-hover".
::: browser/themes/shared/identity-block/identity-icon.svg:11
(Diff revision 2)
> - width="48" height="16" viewBox="0 0 32 16">
> + width="64" height="16" viewBox="0 0 64 16">
> <defs>
> - <circle id="shape-circle-base" cx="8" cy="8" r="7" />
> - <g id="shape-i">
> - <circle cx="8" cy="5" r="1" />
> - <rect x="7" y="7" width="2" height="5" rx="1" ry="1" />
> + <style>
> + path {
> + fill-rule: evenodd;
> + fill:#999999;
nit: missing space
Assignee | ||
Comment 15•8 years ago
|
||
>
> Is it possible to have the ":active" state without the ":hover" state? In
> other words, I wonder if the new selector is fixing an existing issue with
> the hover state. If not, you can just remove it for simplicity.
>
Yeah, I think I just forgot to remove that, good catch!
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8761181 [details]
Bug 1206246 - Change identity icon for sites with granted permissions.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58488/diff/2-3/
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 18•8 years ago
|
||
Keeping track of the bug that implemented the back-end API as a dependency.
Depends on: 1206245
Comment 19•8 years ago
|
||
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c2ffbe222854
Change identity icon for sites with granted permissions. r=paolo
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 21•8 years ago
|
||
I managed to test this bug on 50.0a1 (2016-06-13)
using
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86
The fix has landed and it works as expected, so I am marking this issue as Verified Fixed.
Status: RESOLVED → VERIFIED
Comment 22•8 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: for the first time a user can tell from a quick glance whether the site has been granted any special permissions
[Suggested wording]: glance at the AwesomeBar to see if any special permissions have been granted to the current site
[Links (documentation, blog post, etc)]: none, unfortunately
relnote-firefox:
--- → ?
Comment 23•8 years ago
|
||
I don't think noting this now will be useful since we shipped the feature 6 months ago.
You need to log in
before you can comment on or make changes to this bug.
Description
•