1174284 - SVG images without a specified width and height don't work in native menus

Status: NEW

(Core :: Widget: Cocoa, enhancement, P4)

Description

[Justin Dolske [:Dolske]]

Followup from bug 366324, where I made SVG favicons work.

However, they don't seem to work in OS X native menus (ie, from the History and Bookmarks menu). They're fine in the non-native menus (ie, History/Bookmarks menupanel subviews, awesomebar), so this seems specific to the native bits.

Just to eliminated favicons and Places from the equation, I did a quickie patch to replace the Pocket menuitem in the bookmarks menu with an SVG icon. It also doesn't work. Upon opening the bookmarks menu, I see the following console error:

[GFX3-]: Surface width or height <= 0!
[GFX3-]: Surface width or height <= 0!
Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Failed to allocate a surface due to invalid size Size(-1,-1)|[1][GFX1-]: Failed to allocate a surface due to invalid size Size(-1,-1)[GFX1-]: Failed to allocate a surface due to invalid size Size(-1,-1)
[Parent 575] ###!!! ASSERTION: Could not create a DrawTarget: 'Error', file /Users/dolske/build/mozilla-central/image/VectorImage.cpp, line 701

It would be good to fix this (1) so SVG favicons work correctly and (2) so Firefox and addons can reliably use SVG icons in menus.

[I think we could actually work around #1 by using the existing ability of Places to "optimize" favicons by reencoding them into PNGs, but that wouldn't help with #2.]

I see SVG was made to work with custom cursors (!) in bug 888689, so hopefully this isn't rocket science for menus?

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Testcase

Here's a little testcase that replaces the "View Pocket List" menuitem in the bookmarks menu with an existing SVG icon. I see no icon in the native bookmark menu, and the expected "(i)" icon in the bookmark button non-native menu.

Comment 2

[Tanvi Vyas[:tanvi]]

Since there are no "p4s", what priority is this bug?

Comment 3

[Timothy Nikkel (:tnikkel)]

The problem is with svg images that don't have a usable width or height because their width/height is specified as a percent. SVG images with a specific height work fine.

I can substitute 16 for the width/height (kIconWidth/kIconHeight in nsMenuItemIconX) when we can't get a width/height from vector images, but that won't handle retina screens. Markus do you know how I can get the size that menu icons should be?

Does this work on other platforms already? If not, they would need a platform-specific fix.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Testcase

New testcase that works on trunk. (The frontend code has changed.)

Comment 5

[:Paolo Amadini]

On Mac OS X, at some point we used SVGs that specified 32x32 pixels explicitly, which work fine on HiDPI, and accepted a little blurriness on non-HiDPI.

It would be best if we could get the platform to render the SVG at whatever native resolution we need for the icon. If we do that, likely we should make sure it works also when moving windows across multiple monitors with different HiDPI modes.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to :Paolo Amadini from comment #5)
> It would be best if we could get the platform to render the SVG at whatever
> native resolution we need for the icon.

Yeah, this shouldn't be a problem, I just need to know how to get what that resolution is.

> If we do that, likely we should make
> sure it works also when moving windows across multiple monitors with
> different HiDPI modes.

Note that these icons aren't in *any* window, they are only in menu bar menus. Which stay on one screen unless the set of attached displays changes, or the user moves the menu bar in system preferences. It doesn't look like we have code to detect when that happens currently (unless the whole menu structure is torn down and recreated, I'm not familiar with the mac native code) so currently with raster images this would be an existing problem already.

Comment 7

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> I can substitute 16 for the width/height (kIconWidth/kIconHeight in
> nsMenuItemIconX) when we can't get a width/height from vector images, but
> that won't handle retina screens. Markus do you know how I can get the size
> that menu icons should be?

You could use nsCocoaUtils::CreateNSImageFromImageContainer, and modify it to not take a scale factor but instead always create an NSImage with two representations: one NSImageRep for @1x and one for @2x. The system will automatically pick the one whose resolution matches the target when drawing. (An NSImageRep's resolution is expressed using its pixelsWide/pixelsHigh properties (in image pixels) vs its size property (in user space units).)

We already use nsCocoaUtils::CreateNSImageFromImageContainer for cursors, and HiDPI SVG cursors work, but only because nsChildView::SetCursor passes along its own scale factor.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange [:mstange] from comment #7)
> (In reply to Timothy Nikkel (:tnikkel) from comment #3)
> > I can substitute 16 for the width/height (kIconWidth/kIconHeight in
> > nsMenuItemIconX) when we can't get a width/height from vector images, but
> > that won't handle retina screens. Markus do you know how I can get the size
> > that menu icons should be?
> 
> You could use nsCocoaUtils::CreateNSImageFromImageContainer, and modify it
> to not take a scale factor but instead always create an NSImage with two
> representations: one NSImageRep for @1x and one for @2x. The system will
> automatically pick the one whose resolution matches the target when drawing.
> (An NSImageRep's resolution is expressed using its pixelsWide/pixelsHigh
> properties (in image pixels) vs its size property (in user space units).)
> 
> We already use nsCocoaUtils::CreateNSImageFromImageContainer for cursors,
> and HiDPI SVG cursors work, but only because nsChildView::SetCursor passes
> along its own scale factor.

It looks like we recreate the icon images every time we open the menu. So we'd only ever need one of the two sizes, the other would be a waste. Is there a way to just get the size we currently need? Just being able to get the backing scale factor of the screen the menu is on would be fine, we can multiply 16 by the scale factor. If not, we can use the approach you outlined.

Comment 9

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> (In reply to Markus Stange [:mstange] from comment #7)
> > (In reply to Timothy Nikkel (:tnikkel) from comment #3)
> It looks like we recreate the icon images every time we open the menu.

Oh. Maybe we should do something about that, then. For example caching using an nsExpirationTracker.

> So we'd only ever need one of the two sizes, the other would be a waste.

Is it enough of a waste to be worth worrying about?

> Is there a way to just get the size we currently need?

I'm not sure. I think you can have the main menu on multiple screens, I don't know if there's a way to find out what screen the menu is currently open on. It doesn't have to be the one that the window is on either.
Just always creating both sizes feels less magical and should be harder to get wrong.

Comment 10

[Jonathan Kingston [:jkt] he/him]

> It looks like we recreate the icon images every time we open the menu.

Should this be looked into in terms of perf for Quantum?

Comment 11

[Timothy Nikkel (:tnikkel)]

Link to the relevant code for reference

https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/widget/cocoa/nsMenuItemIconX.mm#332