Open Bug 1316919 Opened 3 years ago Updated 2 years ago

regression: container icons missing from the file menu under OSX

Categories

(Firefox :: Security, defect, P4)

52 Branch
x86
macOS
defect

Tracking

()

Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fix-optional
firefox53 --- wontfix
firefox57 --- fix-optional

People

(Reporter: kjozwiak, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(1 file)

Attached image 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
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.
Has STR: --- → yes
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.
Priority: -- → P1
Flags: in-testsuite?
(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.
Flags: needinfo?(tanvi)
Flags: needinfo?(jkt)
Assignee: nobody → jkt
Flags: needinfo?(jkt)
(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.
Flags: needinfo?(tanvi)
(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.
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!
Flags: needinfo?(paolo.mozmail)
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?
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
Looks like there's little we can do, we likely need SVGs with colors for the menus.
Flags: needinfo?(paolo.mozmail)
Containers are only enabled in nightly, setting 52 as unaffected.
Last time I checked (bug 1174284), SVG didn't work in native menus. Is this the problem here?
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.
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?
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.
baku, can you try adding a width and height and see if that helps?  Kamil can assist with OSX access.
Assignee: jkt → amarchesini
I'm a bit busy right now. jkt, can you take this bug?
Assignee: amarchesini → jkt
Too late for a fix for 53, as we are in the last week of the 53 beta cycle.
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?
Flags: needinfo?(jkt)
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.
Flags: needinfo?(jkt)
Priority: P1 → P4
Component: DOM: Security → Security
Product: Core → Firefox
You need to log in before you can comment on or make changes to this bug.