1267617 - Move notification anchors to the identity block

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[:Paolo Amadini]

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.

Comment 1

[Johann Hofmann [:johannh]]

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.

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1267617 - Move notification anchors to the identity block.

Review commit: https://reviewboard.mozilla.org/r/62096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62096/

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Permission anchors

A GIF that shows permission notifications with the new concept

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Addon Prompt

This is the new icon for addon prompts. It's not very prominent, to say the least.

Comment 5

[Florian Quèze [:florian] (on leave until October 9)]

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/[e-mail]") 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.

Comment 6

[Johann Hofmann [:johannh]]

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/

Comment 7

[Johann Hofmann [:johannh]]

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.

Comment 8

[Florian Quèze [:florian] (on leave until October 9)]

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 9

[Johann Hofmann [:johannh]]

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/

Comment 10

[Johann Hofmann [:johannh]]

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 :)

Comment 11

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f674d8f1902

Comment 13

[Florian Quèze [:florian] (on leave until October 9)]

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.

Comment 14

[Johann Hofmann [:johannh]]

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/

Comment 15

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=daee7ddcb4da

Comment 16

[Johann Hofmann [:johannh]]

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/

Comment 17

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff229b1e69af

Comment 18

[Florian Quèze [:florian] (on leave until October 9)]

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 19

[Johann Hofmann [:johannh]]

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 20

[Florian Quèze [:florian] (on leave until October 9)]

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).

Comment 21

[Johann Hofmann [:johannh]]

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/

Comment 22

[Johann Hofmann [:johannh]]

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 23

[Florian Quèze [:florian] (on leave until October 9)]

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.

Comment 24

[Johann Hofmann [:johannh]]

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/

Comment 25

[Johann Hofmann [:johannh]]

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 26

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3261682bc53

Comment 27

[Johann Hofmann [:johannh]]

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 28

[Florian Quèze [:florian] (on leave until October 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! :-)

Comment 29

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/fx-team/rev/3705eb1dc2cce4582cc3f95af52aeec700801fae
Bug 1267617 - Move notification anchors to the identity block. r=florian

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3705eb1dc2cc

Comment 31

[Paul Silaghi, QA [:pauly]]

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".