1403157 - Permissions warning in menu is not aligned correctly.

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[Sevaan Franks [:sevaan]]

Attachment: Screenshot of issue

The icons in the permissions warning that appears in the menu is not aligned with the rest of the menu.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: minor visual inconsistency with new feature for 57

@bob, can you help get this on someone's radar?

Comment 2

[Andy McKay]

Mark is the best person for that, given the likelihood of this situation occurring, P5 is about the best priority.

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that this is triaged as P5, I don't think we will fix it in 57. Untracked for the same reason.

Comment 5

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Linux before and after comparison gif with guide lines

While the difference isn't as pronounced on linux it is still noticeable.  Seems like the restart banner got some photon re-styling the addon banner missed out on.  Got a patch which brings the styling more in line.  The image showing the issue shows a font variation that doesn't exist on a local build, but that I'm guessing is caused by the addon banner not getting font: menu like the restart one is, so I've added that.
Sevaan which platform are you on, and are you able to test patches?  If not I can make a try build for you.

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Bug 1403157 - Unify appMenu webext new permissions notification appearance with browser update notification.

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

Comment 7

[Sevaan Franks [:sevaan]]

Comment on attachment 8913489 [details]
Linux before and after comparison gif with guide lines

I'm on Mac, and am unable to build. Not sure if a build would help as I already updated the permissions I was being warned about.

Comment 8

[Tim Nguyen :ntim]

Ian, can you also fix the inconsistent font-size on macOS ? See bug 1403887 for the screenshot.

Comment 9

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Tim Nguyen :ntim from comment #8)
> Ian, can you also fix the inconsistent font-size on macOS ? See bug 1403887
> for the screenshot.
I _think_ I did in the patch, after noticing a different font size in the reporter's image, which doesn't present on mine (linux).  Inspecting the styles I didn't notice any font-size difference, but I did notice the restart banner had font: menu while the addon banner only had font: message-box;, and rectified that in my patch.  Is it still different with the patch applied?

(In reply to Mike de Boer [:mikedeboer] from bug 1403887 comment #2)
> (In reply to :Gijs from comment #1)
> > Can you provide steps to reproduce?
> 
> IOW, is there a programmatic way to trigger this state through the Browser
> Console?
I've been installing https://addons.mozilla.org/en-US/firefox/addon/justintv-stream-notificatio/versions/beta?page=1#version-3.5.0pre06 because I know the next version adds a new origin permission, setting extensions.update.interval to 60 and restarting the browser to get the addon banner to show, then doing the equivalent of

let banner = document.querySelector(".panel-banner-item");
banner.removeAttribute("hidden");
banner.setAttribute("notificationid", "update");
banner.setAttribute("label", "Restart to update Nightly");

with the inspector to get the restart banner to show.

(There's probably a more efficient way to trigger an addon update check with the console)

Comment 10

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Ian Moody [:Kwan] from comment #9)
> let banner = document.querySelector(".panel-banner-item");
> banner.removeAttribute("hidden");
> banner.setAttribute("notificationid", "update");
> banner.setAttribute("label", "Restart to update Nightly");
Although banner needs to be

let banner = document.querySelector("#appMenu-mainView").querySelector(".panel-banner-item")

because the old panelMenu is still in the source (and querySelector("#appMenu-mainView .panel-banner-item") doesn't work because XUL, I guess. I was just replacing the hidden attr with the two new ones directly in the inspector anyway)

Comment 11

[Tim Nguyen :ntim]

Attachment: Screenshot of patch on macOS

The font size and the right margin seem fixed thanks!

The left margin still doesn't look quite right though.

Comment 12

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8913490 [details]
Bug 1403157 - Unify appMenu webext new permissions notification appearance with browser update notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184832/diff/1-2/

Comment 13

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Tim Nguyen :ntim from comment #11)
> Created attachment 8914104 [details]
> Screenshot of patch on macOS
> 
> The font size and the right margin seem fixed thanks!
> 
> The left margin still doesn't look quite right though.

Ah, blooming platform specific styles. Think I've found the culprit:
https://searchfox.org/mozilla-central/rev/c296ed9811319cdd61ac35e9e648f95639cda726/browser/themes/osx/customizableui/panelUI.css#24
Fortunately can't see anything in the linux or windows files I might need to account for.

Does the new patch fix it?

Comment 14

[Tim Nguyen :ntim]

Attachment: Screenshot of patch on macOS

Works well, thanks!

Comment 15

[Mark Striemer [:mstriemer]]

Comment on attachment 8913490 [details]
Bug 1403157 - Unify appMenu webext new permissions notification appearance with browser update notification.

https://reviewboard.mozilla.org/r/184832/#review191106

Redirecting to jaws since I'm not very familiar with the toolbar code. Changes look reasonable to me and it looks good on OS X.

::: browser/themes/shared/customizableui/panelUI.inc.css:788
(Diff revision 2)
>    margin-bottom: 3px;
>    padding-inline-start: 12px;
>  }
>  
> +#appMenu-addon-banners > .addon-banner-item {
> +  padding-inline-start: 12px;

It looks like `.addon-banner-item` is only used on the message you're updating. This is overwriting a `padding-inline-start: 15px` above on line 703.

Can you update that rule instead?

Comment 16

[Mark Striemer [:mstriemer]]

I don't see any Windows styles but did you check it on Windows, too?

Comment 17

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8913490 [details]
Bug 1403157 - Unify appMenu webext new permissions notification appearance with browser update notification.

https://reviewboard.mozilla.org/r/184832/#review191106

> It looks like `.addon-banner-item` is only used on the message you're updating. This is overwriting a `padding-inline-start: 15px` above on line 703.
> 
> Can you update that rule instead?

Ah, that's because I was still trying to avoid affecting the old PanelUI, before I found out it doesn't matter anymore.
I can, however it'll then be overridden, first to 0 on line 762, then back to 15 again on line 914.  I think removing the .addon-banner-item selector from the 914 block is probably the best route for that, until all the #PanelUI code goes entirely (or just removing that whole block right now).  As for the 762 block, moving the 703 block to after it, so it's next to the .panel-banner-item block?

(I'm also not sure what the flex stuff still accomplishes on .addon-banner-item)

Comment 18

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Patch on Win7 Classic & Aero

(In reply to Mark Striemer [:mstriemer] from comment #16)
> I don't see any Windows styles but did you check it on Windows, too?

I have now!  Fixed on both Classic and Aero themes on Win7.  (Is there any value to testing Windows 10 as well? Might be doable if need be, just be more difficult)

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8913490 [details]
Bug 1403157 - Unify appMenu webext new permissions notification appearance with browser update notification.

https://reviewboard.mozilla.org/r/184832/#review192016

Comment 20

[(PTO June 19-July 7) Ryan VanderMeulen]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 21

[Ian Moody [:Kwan] (UTC+0)]

Sorry, I'd forgotten about that/got it mixed up with another bug. Done.
(Dropped the issue, since it wasn't as straightforward as first thought, and the r+ didn't mention it)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b226a2f626e2
Unify appMenu webext new permissions notification appearance with browser update notification. r=jaws

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b226a2f626e2

Comment 24

[CosminB]

Attachment: Bug1403157.zip

I can reproduce this issue on Firefox 57.0.1 Dev Edition(20171114132053) under Wind 7 64-bit.

The permissions icon is not aligned correctly with the rest of the icons.

This issue is verified as fixed on Firefox 59.0a1 (20171119220126) and Firefox 58.0b4 Dev Edition (20171115114231) under Wind 7 64-bit and Mac OS X 10.13. 

Please see the attached screenshot.