:Paolo Amadini Reporter
	Description • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 1 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 2 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 3 • 8 years ago

A GIF that shows permission notifications with the new concept

	Johann Hofmann [:johannh] Assignee
	Comment 4 • 8 years ago

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

	Florian Quèze [:florian]
	Comment 5 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 6 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 7 • 8 years ago

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.

	Florian Quèze [:florian]
	Comment 8 • 8 years ago

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?

	Johann Hofmann [:johannh] Assignee
	Comment 9 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 10 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 11 • 8 years ago

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

	Florian Quèze [:florian]
	Comment 13 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 14 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 15 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 16 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 17 • 8 years ago

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

	Florian Quèze [:florian]
	Comment 18 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 19 • 8 years ago

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/

	Florian Quèze [:florian]
	Comment 20 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 21 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 22 • 8 years ago

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.

	Florian Quèze [:florian]
	Comment 23 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 24 • 8 years ago

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/

	Johann Hofmann [:johannh] Assignee
	Comment 25 • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 26 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 27 • 8 years ago

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/

	Florian Quèze [:florian]
	Comment 28 • 8 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 29 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

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

	Paul Silaghi, QA [:pauly]
	Comment 31 • 8 years ago

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