Permissions warning in menu is not aligned correctly.

VERIFIED FIXED in Firefox 58

Status

defect
P5
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: sevaan, Assigned: Kwan, Mentored)

Tracking

({good-first-bug})

57 Branch
mozilla58

Firefox Tracking Flags

(firefox57- wontfix, firefox58 verified, firefox59 verified)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Posted image Screenshot of issue
The icons in the permissions warning that appears in the menu is not aligned with the rest of the menu.
[Tracking Requested - why for this release]: minor visual inconsistency with new feature for 57

@bob, can you help get this on someone's radar?
Component: Menus → WebExtensions: Frontend
Flags: needinfo?(bob.silverberg)
Product: Firefox → Toolkit
Priority: -- → P5
Mark is the best person for that, given the likelihood of this situation occurring, P5 is about the best priority.
Assignee: nobody → mstriemer
Mentor: mstriemer
Flags: needinfo?(bob.silverberg)
Keywords: good-first-bug
Given that this is triaged as P5, I don't think we will fix it in 57. Untracked for the same reason.
Duplicate of this bug: 1403887
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.
Assignee: mstriemer → moz-ian
Status: NEW → ASSIGNED
Flags: needinfo?(sfranks)
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.
Flags: needinfo?(sfranks)
Ian, can you also fix the inconsistent font-size on macOS ? See bug 1403887 for the screenshot.
(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)
Flags: needinfo?(ntim.bugs)
(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)
Posted image Screenshot of patch on macOS (obsolete) —
The font size and the right margin seem fixed thanks!

The left margin still doesn't look quite right though.
Flags: needinfo?(ntim.bugs)
Attachment #8914104 - Attachment description: MacOS screenshot → Screenshot of patch on macOS
(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?
Flags: needinfo?(ntim.bugs)
Works well, thanks!
Attachment #8914104 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8913490 - Flags: review?(mstriemer) → review?(jaws)
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?
I don't see any Windows styles but did you check it on Windows, too?
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)
(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 on attachment 8913490 [details]
Bug 1403157 - Unify appMenu webext new permissions notification appearance with browser update notification.

https://reviewboard.mozilla.org/r/184832/#review192016
Attachment #8913490 - Flags: review?(jaws) → review+
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
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)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b226a2f626e2
Unify appMenu webext new permissions notification appearance with browser update notification. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b226a2f626e2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Posted file 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.
Status: RESOLVED → VERIFIED
See Also: → 1430209
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.