Add to addressbar in the Page Actions menu gives a blank icon when you pin "Add Search Engine"

RESOLVED FIXED in Firefox 66

Status

()

defect
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: asa, Assigned: adw)

Tracking

({regression})

Trunk
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

5 months ago
Tested on latest x86 Nightly build on Windows 10

If you try to pin "Add Search Engine" to the addressbar from the Page Actions menu, you get a blank icon. It functions but it's blank. 

Steps to reproduce
1. Visit ecosia.com or any other page with a search engine you haven't installed (Bugzilla works too)
2. Click the Page Actions menu
3. Right click on "Add Search Engine" and select "Add to Addressbar"

Results: It gets added to the addressbar with no icon

Expected: We get some kind of icon in the addressbar.
WFM on Mac (2019-01-06) - I get the favicon of the Ecosia page.

It works when I do it on the page having the search engine, if then I remove the added search engine and reload the page with the engine, I indeed get an empty space, as reported. Switching tab makes the icon appear. Something fishy.
Drew may know more about how this icon is set, maybe it's a timing problem, maybe the icon src is added when the button is not yet visible and we don't paint it.

Flags: needinfo?(adw)
Priority: -- → P2
Assignee

Comment 3

4 months ago

The icon is set here: https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/base/content/browser-pageActions.js#1124

It looks like this.engines[0].icon is null when _updateTitleAndIcon is called via updateOpenSearchBadge, at least sometimes. The stack I'm seeing when it's null is:

_updateTitleAndIcon@chrome://browser/content/browser-pageActions.js:1139:11
updateEngines@chrome://browser/content/browser-pageActions.js:1128:7
updateOpenSearchBadge@chrome://browser/content/browser.js:3966:5
addEngine@chrome://browser/content/browser.js:3956:9
addSearch@chrome://browser/content/browser.js:3745:5
receiveMessage@chrome://browser/content/browser.js:3675:9
MessageListener.receiveMessageinit@chrome://browser/content/browser.js:3650:5
onLoad@chrome://browser/content/browser.js:1325:5
EventHandlerNonNull
@chrome://browser/content/browser.xul:108:19

It's easy to reproduce: Visit a site that offers an engine, add the "Add Search Engine" action to the urlbar, and reload the page. That triggers the stack above.

It's possible this has always been a problem, but it might be a regression, I don't know.

It looks like the icon comes from browser.mIconURL: https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/base/content/browser.js#3977 So we'd need to somehow wait until that becomes non-null and then set the action's icon.

This isn't a problem for the search bar because the icon is only shown once the user opens the search bar menu, and by that time the icon is likely non-null.

Moving this to Toolbars, where page action bugs live.

Component: Address Bar → Toolbars and Customization
Flags: needinfo?(adw)
Assignee

Comment 4

4 months ago
We can just add `onLinkIconAvailable` to `TabsProgressListener` in browser.js and update the search badge.

This is likely another regression from bug 1453751. Previously the url for the favicon was sent to the browser very early in page load. Now we wait until we've actually loaded the icon's content which is probably after any search engines are detected.

Assignee

Updated

4 months ago
Blocks: 1453751
Assignee

Updated

4 months ago
Assignee: nobody → adw
Status: NEW → ASSIGNED

Comment 6

4 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8dbf2f4f7c53
Update "Add Search Engine" page action image when search engine icon becomes available. r=mossop
Assignee

Updated

4 months ago
Keywords: regression

Comment 7

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
You need to log in before you can comment on or make changes to this bug.