1351255 - puzzle and hamburger icons mis-aligned in install confirmation panel

Status: VERIFIED FIXED

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

Description

[Florian Quèze [:florian]]

Attachment: 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.

Comment 1

[Andrew Swan [:aswan]]

Mark, this is closely related to bug 1329942, do you think you can take it?

Comment 2

[Mark Striemer [:mstriemer]]

Yeah, I can take this.

Comment 3

[Mark Striemer [: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.

Comment 4

[Mark Striemer [:mstriemer]]

Attachment: Bug 1351255 - Align add-on install notification

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

Comment 5

[Mark Striemer [:mstriemer]]

Attachment: windows-updated.png

Comment 6

[Mark Striemer [:mstriemer]]

Attachment: mac-updated.png

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.

Comment 7

[Mark Striemer [:mstriemer]]

Comment on attachment 8862197 [details]
Bug 1351255 - Align add-on install notification

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

Comment 8

[Mark Striemer [:mstriemer]]

Attachment: linux-updated.png

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

Comment 9

[Florian Quèze [:florian]]

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 10

[Florian Quèze [:florian]]

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.

Comment 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8862197 [details]
Bug 1351255 - Align add-on install notification

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

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8862197 [details]
Bug 1351255 - Align add-on install notification

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134086/diff/3-4/

Comment 13

[Mark Striemer [:mstriemer]]

Attachment: mac-aligned.png

I went through and made sure the icons were both visually aligned the same way. Should be good now.

Comment 14

[Mark Striemer [:mstriemer]]

Attachment: linux-aligned.png

Comment 15

[Mark Striemer [:mstriemer]]

Attachment: windows-aligned.png

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

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/[e-mail]");
> +    -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/[e-mail]");
> +    -moz-image-region: rect(0, 972px, 36px, 936px);
> +  }

ditto

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 18

[Mark Striemer [:mstriemer]]

Yes, I'll look into this and bug 1358431 again this week or next.

Comment 19

[Mark Striemer [:mstriemer]]

Attachment: Bug 1351255 - Align icons in webextension install dialog

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

Comment 20

[Mark Striemer [:mstriemer]]

Attachment: windows.png

Comment 21

[Mark Striemer [:mstriemer]]

Attachment: linux.png

Comment 22

[Mark Striemer [:mstriemer]]

Attachment: mac.png

Comment 23

[Mark Striemer [:mstriemer]]

Updated with changes on inbound and new screenshots uploaded.

Comment 24

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 25

[Mark Striemer [:mstriemer]]

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.

Comment 26

[Mark Striemer [:mstriemer]]

Comment on attachment 8873901 [details]
Bug 1351255 - Align icons in webextension install dialog

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/405e89243e0a
Align icons in webextension install dialog r=nhnt11

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/405e89243e0a

Comment 29

[CosminB]

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