1449200 - Missing page actions menu icons

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Mauro Sanabria]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180327105613

Steps to reproduce:

* This is seen either with some webextensions, in safe mode or even in a brand new profile since a few Nightlies back.
* I believe this was OK at the start of the 61 cycle.
* Click on the page actions menu (ellipsis icon in Address Bar).


Actual results:

* While the "Take a Screenshot" and "Report Site Issue..." are available and functional their icons are missing.
* Adding their icons to the Address Bar with the context menu does not change this.
* No other page action is affected.


Expected results:

* The "Take a Screenshot" and "Report Site Issue..." page actions should display their respective icons.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Drew, any idea?

Comment 2

[Drew Willcoxon :adw]

Interesting, they're the only two actions that are provided entirely through extensions.  The only thing that changed with page actions recently is bug 1446250, and it did touch icons.  Kris, could you take a look please?

I can't reproduce the problem though.

Comment 3

[Drew Willcoxon :adw]

Other people are reporting this same problem for the new "add search engine" action, so I'm going to mark this as confirmed even though I haven't confirmed it myself.  I can't reproduce the problem on macOS, so I'm wondering if this only happens on Windows and Linux, but that seems weird.

Comment 4

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #2)
> Interesting, they're the only two actions that are provided entirely through
> extensions.

The fact that people are seeing this with the new "add search engine" action points to this being a problem with all the actions that aren't in browser.xul or don't otherwise add DOM nodes themselves -- i.e., everything but the bookmark star and Pocket.

Comment 5

[Drew Willcoxon :adw]

Er, maybe not.  The other built-in actions specify their icons in CSS.  They don't call action.setIconURL().  The "add search engine" action does call setIconURL().  So maybe it has something to do with that.  Anyway, this needs investigation.

Comment 6

[Drew Willcoxon :adw]

It's something to do with screen DPI.  I'm on a "retina" screen.  No problem.  I set layout.css.devPixelsPerPx=1, problem.

Comment 7

[Mauro Sanabria]

Thanks for tackling this issue; this seems to be in the right track: doing the converse (Fx x64 on Windows 10 x64 laptop, standard panel display) by setting layout.css.devPixelsPerPx=2 from its -1 default makes the icons appear (it's no workaround, of course).

Comment 8

[Mauro Sanabria]

(Sorry for the spam) On the other hand, just noticed the new page action for adding a new search engine and its icon renders properly (at least for Bugzilla and YouTube) in my default conditions (layout.css.devPixelsPerPx=-1).

Comment 9

[Drew Willcoxon :adw]

It probably has to do with the size of the icon, not only your DPI.

I suspect the problem is here: https://dxr.mozilla.org/mozilla-central/rev/6aa3b57955fed5e137d0306478e1a4b424a6d392/browser/modules/PageActions.jsm#736

Comment 10

[:Gijs (he/him)]

Isn't this just a straight dupe of bug 1447943? Though perhaps at this point this bug has more info...

Comment 11

[Drew Willcoxon :adw]

Well, that bug is filed in webextensions...  But yeah, the cause is this bug.

Comment 12

[Drew Willcoxon :adw]

Also, bug 1446250 removed all callers of PageActions.iconURLForSize (the non-underscored one) but not the method itself, so the fix for this bug might as well do that too.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1449200: Return icon URL for all sizes when processing an icon string.

The CSS for page action icons doesn't handle fallback when only one variable
is defined, so for widgets that don't define their icons using CSS, we always
need to provide both.

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

Comment 15

[:Gijs (he/him)]

Comment on attachment 8963824 [details]
Bug 1449200: Return icon URL for all sizes when processing an icon string.

https://reviewboard.mozilla.org/r/232694/#review238174

r=me with Drew's feedback incorporated.

::: browser/modules/PageActions.jsm:819
(Diff revision 1)
>     *
>     * @param  peferredSize (number, required)
>     *         The icon size you prefer.
>     * @return The URL of the best icon, or null.
>     */
>    iconURLForSize(preferredSize, browserWindow) {

As Drew noted:

(In reply to Drew Willcoxon :adw from comment #12)
> Also, bug 1446250 removed all callers of PageActions.iconURLForSize (the
> non-underscored one) but not the method itself, so the fix for this bug
> might as well do that too.



please rm this and move the comment to the underscored method, removing the "@see ..." clause there.

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9052a77c9906a9be02b8442c10346c20fcd486d
Bug 1449200: Return icon URL for all sizes when processing an icon string. r=Gijs

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e9052a77c990