Closed Bug 1267617 Opened 4 years ago Closed 3 years ago

Move notification anchors to the identity block

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox49 --- affected
firefox50 --- verified

People

(Reporter: Paolo, Assigned: johannh)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [fxprivacy])

Attachments

(3 files)

When a permission request is active, it should be anchored to the "i" icon and prevent the Control Center and other permission requests from being opened until the request has been dismissed.
Priority: -- → P2
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P2 → P1
QA Contact: paul.silaghi
Iteration: --- → 50.1
Iteration: 50.1 → 50.2
Whiteboard: [fxprivacy] → [fxprivacy][b][c]
Changing the description of this bug as anchoring at the (i) icon has apparently been discarded for now, because of some technical and conceptual difficulties. The newest concept can be found here: https://mozilla.invisionapp.com/share/9J7RMC2G7#/screens/170569580. I'll upload a patch in a second.
Summary: Anchor permission notifications to the "i" icon → Move permission notifications to the identity block
Summary: Move permission notifications to the identity block → Move notification anchors to the identity block
Attached image Permission anchors
A GIF that shows permission notifications with the new concept
Attached image Addon Prompt
This is the new icon for addon prompts. It's not very prominent, to say the least.
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

https://reviewboard.mozilla.org/r/62096/#review58794

::: browser/themes/shared/notification-icons.inc.css
(Diff revision 1)
> -  background-color: #fff;
>    background-clip: padding-box;
> -  padding-left: 3px;
> -  border-width: 0 8px 0 0;
> -  border-style: solid;
> -  border-image: url("chrome://browser/skin/urlbar-arrow.png") 0 8 0 0 fill;

I think we'll need to make a similar change to browser/themes/shared/devedition.inc.css

Also, it seems you can now remove urlbar-arrow*.png.

::: browser/themes/shared/notification-icons.inc.css:12
(Diff revision 1)
> -  #notification-popup-box {
> -    border-image: url("chrome://browser/skin/urlbar-arrow@2x.png") 0 16 0 0 fill;
> -  }
>  }
>  
>  @conditionalForwardWithUrlbar@ > #forward-button[disabled] + #urlbar > #notification-popup-box {

I think this rule needs to be touched too. Either this was specific to the notification box being right after the forward button and it can be removed, or it was related to the element right after the forward button, which is now #identity-box

Please also check the rules involving #notification-popup-box[hidden] + #identity-box in browser/themes/shared/identity-block/identity-block.inc.css

::: browser/themes/shared/notification-icons.inc.css:16
(Diff revision 1)
>  
>  @conditionalForwardWithUrlbar@ > #forward-button[disabled] + #urlbar > #notification-popup-box {
>    padding-left: calc(var(--backbutton-urlbar-overlap) + 3px);
>  }
>  
>  /* This changes the direction of the main notification box on the url bar. */

I think the point of all these CSS transform was to change the direction of the arrow, so these rules can likely be removed now.
Attachment #8767642 - Flags: review?(florian) → review-
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/1-2/
Attachment #8767642 - Flags: review- → review?(florian)
Fixed comments and made the addons icon gray. Also removed the hover states from the addon and plugin icons, we should consistently decide for or against a hover state on notification anchors, but we can do that as a follow-up.
https://reviewboard.mozilla.org/r/62096/#review58814

::: browser/themes/shared/identity-block/identity-block.inc.css:28
(Diff revision 2)
>                                  var(--urlbar-separator-color) 15%,
>                                  var(--urlbar-separator-color) 85%,
>                                  transparent 85%);
>    border-image-slice: 1;
>    font-size: .9em;
> -  padding: 3px 5px;
> +  padding: 3px 5px 3px 11px;

Should this use padding-inline-begin/end?

::: browser/themes/shared/notification-icons.inc.css:13
(Diff revision 2)
> -@conditionalForwardWithUrlbar@ > #forward-button[disabled] + #urlbar > #notification-popup-box {
> -  padding-left: calc(var(--backbutton-urlbar-overlap) + 3px);
>  }
>  
>  /* This changes the direction of the main notification box on the url bar. */
>  #notification-popup-box:-moz-locale-dir(rtl),

Do we still need this, or did you miss the last line of comment 5?
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/2-3/
https://reviewboard.mozilla.org/r/62096/#review58814

> Should this use padding-inline-begin/end?

I actually made a mistake there, should be fixed now.

> Do we still need this, or did you miss the last line of comment 5?

No, I misread comment 5 :)
Duplicate of this bug: 1267622
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

https://reviewboard.mozilla.org/r/62096/#review58888

Looks good. The one thing that is missing is "the background color hint on hover is removed, as it was hinting to the single click target, which is no longer the case" (I guess this also applies to the darker gray background for the open state).

Will be r+ with that fixed and the tests passing.
Attachment #8767642 - Flags: review?(florian)
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/3-4/
Attachment #8767642 - Flags: review?(florian)
Whiteboard: [fxprivacy][b][c] → [fxprivacy]
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/4-5/
https://reviewboard.mozilla.org/r/62096/#review59140

::: browser/themes/shared/identity-block/identity-block.inc.css
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  %endif
>  
>  #identity-box {
> -  --identity-box-hover-background-color: rgb(231,230,230);
> -  --identity-box-selected-background-color: rgb(211,210,210);

Similar rules to remove at:
http://searchfox.org/mozilla-central/source/browser/themes/osx/browser.css#1579
and
http://searchfox.org/mozilla-central/source/browser/themes/shared/devedition.inc.css#64
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/5-6/
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

https://reviewboard.mozilla.org/r/62096/#review59164

::: browser/themes/osx/browser.css:2976
(Diff revision 6)
>  }
>  
>  %include ../shared/notification-icons.inc.css
>  
>  #notification-popup-box {
>    border-radius: 2px 0 0 2px;

I think we should remove this rule (and same comment for Linux and Windows).

When testing the patch it's unfortunate that the (i) hover effect is triggered on the area at the top/bottom of the notification icons, and between the notification icons and the url bar separator when there's no lock icon.

I think we could 'fix' this by adding rules like:
padding-top: 3px;
margin-top: -3px; (if needed to prevent expending the location bar's height)
(same for *-bottom)

And when the lock isn't shown we could add:
margin-inline-end: -5px;
padding-inline-end: 5px;

In my quick testing with DOM Inspector these rules improved the situation, but didn't fix it completely (like if there was somehow a 1px border I couldn't find that still triggered the hover effect).
Attachment #8767642 - Flags: review?(florian)
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/6-7/
Attachment #8767642 - Flags: review?(florian)
Some explanations:

No idea why, but using 5px instead of 3px for the additional vertical padding makes the 1px border problem go away. (I know it's because the identity box has a height of 20 instead of 16, but I don't know where that height comes from).

The right hand side should work as expected now, both with and without the icon, I'd appreciate if you could try it for yourself :)

Note that there is still the right hand border which triggers a hover on the (i). Thinking about it, we could use a css :last-child selector to give that border to the last element instead.
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

https://reviewboard.mozilla.org/r/62096/#review59348

(In reply to Johann Hofmann [:johannh] from comment #22)

> Note that there is still the right hand border which triggers a hover on the
> (i). Thinking about it, we could use a css :last-child selector to give that
> border to the last element instead.

We can try moving the border to the urlbox.

::: browser/base/content/browser.xul:731
(Diff revision 7)
> +                   ondragstart="gIdentityHandler.onDragStart(event);">
> +                <image id="identity-icon"
> +                       consumeanchor="identity-box"
> +                       onclick="PageProxyClickHandler(event);"/>
> +                <box id="notification-popup-box"
> +                     hidden="true"

Let's add tooltiptext="" to prevent the identity tooltip from being shown when hovering the permission icons.

::: browser/themes/linux/browser.css
(Diff revision 7)
>  
>  .addon-install-confirmation-name {
>    font-weight: bold;
>  }
>  
> -/* Notification icon box */

This comment is still relevant to the next rule.

::: browser/themes/windows/browser.css
(Diff revision 7)
>  
>  .addon-install-confirmation-name {
>    font-weight: bold;
>  }
>  
> -/* Notification icon box */

Same here.
Attachment #8767642 - Flags: review?(florian)
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/7-8/
Attachment #8767642 - Flags: review?(florian)
This should cover it, I hope. Note that with this last step I modified some cross-platform styles, so we should check that this works everywhere. I made another mozscreenshots try push.
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62096/diff/8-9/
Comment on attachment 8767642 [details]
Bug 1267617 - Move notification anchors to the identity block.

https://reviewboard.mozilla.org/r/62096/#review59390

Thanks for the fast iterations here; r=me, finally! :-)
Attachment #8767642 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/3705eb1dc2cc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Verified fixed FX 50.0a1 (2016-07-10) Win 7, Ubuntu 14.04, OS X 10.9.5.
Note that it's a little odd now on a https page when the green lock icon is distanced from the "i" icon due to several permission icons between them and clicking the lock icon opens the control center under "i".
Status: RESOLVED → VERIFIED
Blocks: 1235838
Depends on: 1327539
Depends on: 1327940
Depends on: 1327946
Depends on: 1333437
No longer depends on: 1333437
Depends on: 1345693
You need to log in before you can comment on or make changes to this bug.