1316919 - regression: container icons missing from the file menu under OSX

Status: RESOLVED WORKSFORME

(Firefox :: Security, defect, P4)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: issue.png

It looks like the work that was done in bug#1278177 regressed the container icons in the file menu under macOS.

Platforms affected:

* macOS 10.12.1 - affected
* Ubuntu 16.04.1 LTS - not affected
* Win 10 x64 - not affected

14:13.55 INFO: Last good revision: f19157ac2e4800139a5b82896a7fa0b0846469d6
14:13.55 INFO: First bad revision: ab12ade757276792c6e077e27563ce3cc5a145c0
14:13.55 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f19157ac2e4800139a5b82896a7fa0b0846469d6&tochange=ab12ade757276792c6e077e27563ce3cc5a145c0

14:14.11 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1278177

Comment 1

[Kamil Jozwiak [:kjozwiak]]

STR:

* launch the latest version of m-c
* click on "File -> New Container Tab"

You'll notice that the container icons are not being displayed.

Comment 2

[Tanvi Vyas[:tanvi]]

Again :(

We really need to add browser chrome tests or mozscreenshots this time.

We probably also need to move the component here from Dom:Security to Firefox:something.  Perhaps jkt can find the right component, or ask someone who knows.

Comment 3

[Andrew Overholt [:overholt]]

(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Again :(
> 
> We really need to add browser chrome tests or mozscreenshots this time.
> 
> We probably also need to move the component here from Dom:Security to
> Firefox:something.

Do we have a containers component under Firefox yet?

> Perhaps jkt can find the right component, or ask someone
> who knows.

I'll needinfo jkt.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Andrew Overholt [:overholt] from comment #3)
> (In reply to Tanvi Vyas [:tanvi] from comment #2)
> > Again :(
> > 
> > We really need to add browser chrome tests or mozscreenshots this time.
> > 
> > We probably also need to move the component here from Dom:Security to
> > Firefox:something.
> 
> Do we have a containers component under Firefox yet?
> 
No.  Should we create one, or is it too early?  I'm not sure "Containers" will be called "Containers" in the end.  We are about to go through a Test Pilot study to see how users like it and how we need to modify the UI that we have in Nightly.

Comment 5

[Andrew Overholt [:overholt]]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> (In reply to Andrew Overholt [:overholt] from comment #3)
> > (In reply to Tanvi Vyas [:tanvi] from comment #2)
> > > Again :(
> > > 
> > > We really need to add browser chrome tests or mozscreenshots this time.
> > > 
> > > We probably also need to move the component here from Dom:Security to
> > > Firefox:something.
> > 
> > Do we have a containers component under Firefox yet?
> > 
> No.  Should we create one, or is it too early?

I don't think it's too early and we can always disable it if we find it useless. The name change could be tricky if people have saved queries but maybe Bugzilla can support redirects. I'd ask :dkl.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Hey I think it was you who suggested to using your filters.svg, the problem being here is that it appears OSX doesn't permit anything other than list-style-image in the top navigation menus.

Is there a way to make filters.svg work with list-style-image or do you know why background might not be working in these menus? I have tried various things in the CSS but nothing seems to change it.

Will keep looking otherwise.

Thanks!

Comment 7

[:Paolo Amadini]

I don't think I specifically suggested using "filters.svg" for menu icons, but SVG in general should work correctly, and the 32px size we used for the icons should work with HiDPI menus too.

This worked when we first implemented it. Maybe the regression is to be found in some later change?

Comment 8

[Jonathan Kingston [:jkt] he/him]

There have been various regressing changes for this icon, one was in docshell permisisons.
The regressing change this time was moving the icons to using backgrounds with the filters so they could be dynamically assigned, I initially implemented it with list-style-image with multiple svg and anchors for colours however the single svg and filters was a much more elegant solution however this requires a background rather than list-style-image to be implemented. This however doesn't appear to work with those native-ish? menus at the top of OSX.

Comment 9

[:Paolo Amadini]

Looks like there's little we can do, we likely need SVGs with colors for the menus.

Comment 10

[Julien Cristau [:jcristau]]

Containers are only enabled in nightly, setting 52 as unaffected.

Comment 11

[Justin Dolske [:Dolske]]

Last time I checked (bug 1174284), SVG didn't work in native menus. Is this the problem here?

Comment 12

[Jonathan Kingston [:jkt] he/him]

Looks like it however it was working when the images were set as a list-style-image rather than background. However it could be the addition of filters for the colours. I haven't had time to debug.

Comment 13

[Tanvi Vyas[:tanvi]]

If SVG icons aren't going to work, can we add a set of pngs too?  Or somehow convert svg to png and then show them in the menu?

Comment 14

[:Paolo Amadini]

SVG works, see comment 7. Likely, SVG filters don't work.

Bug 1174284 comment 3 mentions that SVG without a set width and height doesn't work.

Comment 15

[Tanvi Vyas[:tanvi]]

baku, can you try adding a width and height and see if that helps?  Kamil can assist with OSX access.

Comment 16

[Andrea Marchesini [:baku]]

I'm a bit busy right now. jkt, can you take this bug?

Comment 17

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

Too late for a fix for 53, as we are in the last week of the 53 beta cycle.

Comment 18

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

Jonathan, can you take a look and let me know whether this is imortant? It's still marked as P1. Should we lower that rating?

Comment 19

[Jonathan Kingston [:jkt] he/him]

Changing to P4 for the time being this is super annoying but not broken in that we are not seeing the black icon issue that we have had elsewhere. I think this depends on a filter issue that impacts this and unless we are to refactor how the CSS works we can only wait on that.

Comment 20

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 21

[Dan Mehic]

Verified on Firefox Nightly 128.0a1 on macOS.