browser action with broken icon doesn't have padding

RESOLVED WONTFIX

Status

()

Toolkit
WebExtensions: Frontend
P2
normal
RESOLVED WONTFIX
7 months ago
2 months ago

People

(Reporter: dietrich, Assigned: mstriemer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

7 months ago
The container should be statically sized, so even when icon is broken (point to wrong file or whatnot), it's visually clear in the add-ons area, instead of making everything squash together.

You can test this by adding a web extension with an icon location pointing to a file that doesn't exist, and adding a badge.

Screenshot:

https://i.imgur.com/v3LvIBo.png

Note how all the icons have proper padding/margin except the badge.

Updated

7 months ago
Assignee: nobody → mstriemer
Priority: -- → P2
Whiteboard: [triaged]
(Assignee)

Comment 1

6 months ago
I made an add-on to test this and it doesn't look great in Firefox. I tried loading it in Chrome and it wouldn't let me since it couldn't load the icon. This seems reasonable to me, if you want the default icon you shouldn't specify one.

I updated mozilla/addons-linter#1026 [1] to suggest we prevent accepting extensions with missing icons.

So that would fix the issue in the future (somewhat) but we should probably handle this in Firefox anyway.

[1] https://github.com/mozilla/addons-linter/issues/1026
Comment hidden (mozreview-request)
(Assignee)

Comment 3

6 months ago
Created attachment 8874609 [details]
toolbar.png

Here's a screenshot with a fix. There are two extensions with badges, one missing the icon in it.

This change is fairly naive, I'm not sure what other areas this might affect but it seems reasonable that we'd always want the image to be 16x16.
Could I get a link to the add-on you made to test this?
(Assignee)

Comment 5

6 months ago
Created attachment 8874855 [details]
missing-icon-badge-0.1.zip
(Assignee)

Comment 6

6 months ago
Created attachment 8874856 [details]
icon-badge-0.1.zip

Comment 7

6 months ago
mozreview-review
Comment on attachment 8874606 [details]
Bug 1368447 - Don't break toolbar layout if browserAction has no icon

https://reviewboard.mozilla.org/r/145950/#review150222

::: browser/themes/shared/toolbarbuttons.inc.css:92
(Diff revision 1)
>  :-moz-any(toolbar, .widget-overflow-list) .toolbarbutton-1 > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon,
>  #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
> +  /* Set min-width and min-height so this is still 16x16 if the image is missing, max-width handles images larger than 16px. */
> +  min-height: 16px;
> +  min-width: 16px;
>    max-width: 16px;

Setting min-width and max-width to the same value doesn't seem to make sense, as you should just use width then.

However, this code is currently using max-width to support icons smaller than 16px. It's not clear to me that we want to stop doing that.
Attachment #8874606 - Flags: review-

Comment 8

6 months ago
mozreview-review
Comment on attachment 8874606 [details]
Bug 1368447 - Don't break toolbar layout if browserAction has no icon

https://reviewboard.mozilla.org/r/145950/#review150226

Looks fine to me, _but_ Dao is far more familiar with the code, so we should address any comments of his before landing.
Attachment #8874606 - Flags: review?(bwinton) → review+
(Assignee)

Comment 9

6 months ago
(In reply to Dão Gottwald [::dao] from comment #7)
> Setting min-width and max-width to the same value doesn't seem to make
> sense, as you should just use width then.

I agree, however setting `width: 16px` breaks the interface entirely [1]. I don't know why this works, but it does. Breaking out the one rule that applies and setting `width` on that gets pretty close, but not quite [2].

> However, this code is currently using max-width to support icons smaller
> than 16px. It's not clear to me that we want to stop doing that.

I don't see us validating the size of icons provided by extensions so I think we're going to have to scale them up or down depending on the resolution no matter what. 

[1] https://www.dropbox.com/s/vd5245e9ps92k6o/Screenshot%202017-06-06%2010.09.55.png?dl=0
[2] https://www.dropbox.com/s/gjseo6hm822xe33/Screenshot%202017-06-06%2010.11.58.png?dl=0
(In reply to Mark Striemer [:mstriemer] from comment #9)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > Setting min-width and max-width to the same value doesn't seem to make
> > sense, as you should just use width then.
> 
> I agree, however setting `width: 16px` breaks the interface entirely [1]. I
> don't know why this works, but it does. Breaking out the one rule that
> applies and setting `width` on that gets pretty close, but not quite [2].

That's because you haven't updated this rule:

http://searchfox.org/mozilla-central/rev/d441cb24482c2e5448accaf07379445059937080/browser/themes/shared/toolbarbuttons.inc.css#184-194

> > However, this code is currently using max-width to support icons smaller
> > than 16px. It's not clear to me that we want to stop doing that.
> 
> I don't see us validating the size of icons provided by extensions so I
> think we're going to have to scale them up or down depending on the
> resolution no matter what.

I'm not sure what you mean here. We're currently scale icons down but not up, and that was a deliberate choice.
(Assignee)

Comment 11

6 months ago
Alright, so I changed the approach which doesn't scale up icons. Now the xul:stack element must be the same width instead of depending on the icon being big enough to hit its max-width.

I updated the styles for in the toolbar and tab bar but without an icon the badge is centred in the tab bar, defining a height didn't seem to work the same way as in the toolbar.

Does this approach seem more reasonable? I can update it for photon styles too but figured I'd check first.

[toolbar] https://www.dropbox.com/s/k37jhwiwu5hfftq/Screenshot%202017-06-06%2014.22.45.png?dl=0
[tab bar] https://www.dropbox.com/s/7o6b033483fb1j3/Screenshot%202017-06-06%2014.23.29.png?dl=0
(Assignee)

Updated

6 months ago
Attachment #8874606 - Flags: review- → review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8874606 - Flags: review?(dao+bmo)

Updated

6 months ago
Attachment #8874606 - Flags: review?(dao+bmo)

Comment 13

6 months ago
mozreview-review
Comment on attachment 8874606 [details]
Bug 1368447 - Don't break toolbar layout if browserAction has no icon

https://reviewboard.mozilla.org/r/145950/#review150632

::: browser/themes/shared/toolbarbuttons.inc.css:186
(Diff revision 2)
>    transition-duration: 150ms;
>  }
>  
> +#nav-bar .toolbarbutton-1 > .toolbarbutton-badge-stack {
> +  width: 28px;
> +  height: 28px;

No, this isn't correct either, as it doesn't take compact and touch modes into account where buttons are smaller or bigger than normal.

Honestly I think we should just wontfix this bug. It seems like an odd thing to worry about. The user experience is going to be bad here no matter what, and ultimately the fix needs to be in the add-on.

We could use resource://gre-resources/broken-image.png as a fallback icon, but that's only going to work if the add-on doesn't set an icon at all rather than a broken one.
Attachment #8874606 - Flags: review-
(Assignee)

Comment 14

6 months ago
Sure, we can leave it as it is and prevent these extensions from being installed in the future.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → WONTFIX
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1411291
You need to log in before you can comment on or make changes to this bug.