Closed Bug 1351255 Opened 3 years ago Closed 2 years ago

puzzle and hamburger icons mis-aligned in install confirmation panel

Categories

(Toolkit :: Add-ons Manager, defect, P5, minor)

defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox55 --- verified
firefox56 --- verified

People

(Reporter: florian, Assigned: mstriemer)

References

Details

(Whiteboard: triaged)

Attachments

(5 files, 7 obsolete files)

Attached image Screenshot
See attached screenshot.

I think these 2 icons should be either baseline aligned, or vertically centered compared to the text. Having these 2 icons floating about the line of text seems wrong.
Priority: -- → P5
Whiteboard: triaged
Keywords: good-first-bug
Mark, this is closely related to bug 1329942, do you think you can take it?
Flags: needinfo?(mstriemer)
Yeah, I can take this.
Assignee: nobody → mstriemer
Flags: needinfo?(mstriemer)
The extension name and icon are not aligned in this popup just like in bug 1329942. That should be fixed while the icons are being aligned.
Attached image windows-updated.png (obsolete) —
Attached image mac-updated.png (obsolete) —
Here are some mac and windows screenshots. Having trouble getting the build on my linux VM. I'll try again tomorrow morning and verify it looks okay.

I just hid the extra description at the top of the popup since it doesn't support HTML and is causing a flash in the permissions popup. I'll update the permissions popup once this one is ready.
Attached image linux-updated.png (obsolete) —
It was looking a little cramped on linux so I added some more padding above the bottom paragraph.

Try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=325a43214339ed0a1249128cfe31f5d73afdef9f
On these screenshots, the alignment of the addon-toolbar-icon icon looks much better, but the alignment of the addon-addon-icon icon still doesn't look great to me. I would expect the bottom of the puzzle icon to be aligned with the baseline of the text (ie. aligned with the bottom of 'clickin' not with the bottom of 'g').
Comment on attachment 8862197 [details]
Bug 1351255 - Align add-on install notification

I'm currently quite busy so I'll forward this request to Nihanth.
Attachment #8862197 - Flags: review?(florian) → review?(nhnt11)
Attached image mac-aligned.png (obsolete) —
I went through and made sure the icons were both visually aligned the same way. Should be good now.
Attachment #8862199 - Attachment is obsolete: true
Attached image linux-aligned.png (obsolete) —
Attachment #8862596 - Attachment is obsolete: true
Attached image windows-aligned.png (obsolete) —
Attachment #8862198 - Attachment is obsolete: true
Comment on attachment 8862197 [details]
Bug 1351255 - Align add-on install notification

https://reviewboard.mozilla.org/r/134086/#review142934

::: browser/themes/linux/browser.css:851
(Diff revision 4)
> +
> +@media (min-resolution: 1.1dppx) {
> +  .addon-toolbar-icon {
> +    list-style-image: url("chrome://browser/skin/Toolbar@2x.png");
> +    -moz-image-region: rect(0, 972px, 36px, 936px);
> +  }

You won't need this block anymore, since we switched to SVG icons. You'll have to rebase the patch though.

::: browser/themes/windows/browser.css:2139
(Diff revision 4)
> +
> +@media (min-resolution: 1.1dppx) {
> +  .addon-toolbar-icon {
> +    list-style-image: url("chrome://browser/skin/Toolbar@2x.png");
> +    -moz-image-region: rect(0, 972px, 36px, 936px);
> +  }

ditto
Attachment #8862197 - Flags: review?(nhnt11)
Mark, still working on this? We could still take this fix in Nightly. It would be nice to fix this and maybe bug 1358431 for 55.
Flags: needinfo?(mstriemer)
See Also: → 1358431
Yes, I'll look into this and bug 1358431 again this week or next.
Flags: needinfo?(mstriemer)
Attachment #8862197 - Attachment is obsolete: true
Attached image windows.png
Attachment #8865925 - Attachment is obsolete: true
Attached image linux.png
Attachment #8865924 - Attachment is obsolete: true
Attached image mac.png
Attachment #8865923 - Attachment is obsolete: true
Updated with changes on inbound and new screenshots uploaded.
Comment on attachment 8873901 [details]
Bug 1351255 - Align icons in webextension install dialog

https://reviewboard.mozilla.org/r/145292/#review151792

Thanks!

Personally I feel that it looks nicer without the `margin-bottom: 1px;` on the icons; however, it seems that was already discussed previously in this bug so I won't let that block this.

::: browser/themes/windows/browser.css:1638
(Diff revision 1)
>    height: 14px;
>    list-style-image: url("chrome://browser/skin/menu.svg");
>    -moz-context-properties: fill;
>    fill: var(--toolbarbutton-icon-fill);
> +  vertical-align: bottom;
> +  margin-bottom: 1px;

Nit: These two rules bring the total number of rules common to .addon-addon-icon and .addon-toolbar-icon to 4. That makes me want to group those together and save some duplication, but that might not be as readable as I suspect once executed.

Please try and deduplicate these rules if you think readability will be preserved.

It's also unfortunate that these identical-across-platforms styles are not defined in a shared CSS file, but maybe that can be deduplicated as part of Photon.
Attachment #8873901 - Flags: review?(nhnt11) → review+
Comment on attachment 8873901 [details]
Bug 1351255 - Align icons in webextension install dialog

https://reviewboard.mozilla.org/r/145292/#review151792

I agree that having them lower looks fine but looking at the address bar it looks like they're somewhere between the bottom of a descender and the baseline. So I think this is about right.

> Nit: These two rules bring the total number of rules common to .addon-addon-icon and .addon-toolbar-icon to 4. That makes me want to group those together and save some duplication, but that might not be as readable as I suspect once executed.
> 
> Please try and deduplicate these rules if you think readability will be preserved.
> 
> It's also unfortunate that these identical-across-platforms styles are not defined in a shared CSS file, but maybe that can be deduplicated as part of Photon.

Combining them looks good to me. I left them in the platform specific stylesheets for now.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/405e89243e0a
Align icons in webextension install dialog r=nhnt11
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/405e89243e0a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
This issue is verified as fixed on Firefox 55.0b1(20170612224034) and 56.0a1(20170614030206) under Wind 7 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.10.4
 
The addon-toolbar-icon icon and addon-addon-icon icon are aligned properly with the text in the confirmation pop-up.
 
Windows - https://www.dropbox.com/s/uz1i9obypf3o8wf/Windows55.0b1.png?dl=0 
Linux - https://www.dropbox.com/s/75ucvk35gx9sgvq/Linux550b1.png?dl=0 
Mac - https://www.dropbox.com/s/mold0hhyb2gnavn/Mac55.0b1.png?dl=0
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.