1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar/address bar/location bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Maverick]

Attachment: Location Bar with blank button

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

Hided "Search Bar" being left only with Address Bar
tested with:

https://searx.me
https://bugzilla.mozilla.org

New search engine is detected and shown on page actions.

Right click on the button/new Search engine, and choose "Add to Address Bar"


Actual results:

Address bar gets a blank space/button
If clicked it does not add the new engine neither does show any dialog(in case of more than one engine)


Expected results:

The "Engine Icon" with the "Plus" sign over it should show up on the address bar.
Upon left-click it should add the new engine or show up a dialogue with the possible engines to add (if more than one)

Comment 1

[:Gijs (he/him)]

I can reproduce the icon in the URL bar not doing anything, and I get:

NS_ERROR_FAILURE: addEngine: Error adding engine:
[Exception... "Engine location is neither a File nor a URI object"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: jar:file:///Applications/FirefoxNightly.app/Contents/Resources/omni.ja!/components/nsSearchService.js :: ERROR :: line 218"  data: no]

on both pages.

However, on both pages the icon shows up correctly for me (macOS, retina display in case that's relevant). Your comment here has a Linux user agent, the comment in bug 1221539 had a Windows one. Are you seeing this on both OSes?

Drew, can you take a look at the error?

Comment 2

[Maverick]

(In reply to :Gijs from comment #1)
> I can reproduce the icon in the URL bar not doing anything, and I get:
> 
> NS_ERROR_FAILURE: addEngine: Error adding engine:
> [Exception... "Engine location is neither a File nor a URI object" 
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> jar:file:///Applications/FirefoxNightly.app/Contents/Resources/omni.ja!/
> components/nsSearchService.js :: ERROR :: line 218"  data: no]
> 
> on both pages.
> 
> However, on both pages the icon shows up correctly for me (macOS, retina
> display in case that's relevant). Your comment here has a Linux user agent,
> the comment in bug 1221539 had a Windows one. Are you seeing this on both
> OSes?
> 
> Drew, can you take a look at the error?

(I'm sorry, this happened because i changed computers...)
Yes. I can confirm this has the same behavior on Windows 10 and openSUSE Tumbleweed. 
In both i don't see any icon (like in the image attached i get that blank space between Pocket and Bookmark's Star).

Still, on page icons the icon is there and i can successfully add the new search engine

Comment 3

[Maverick]

(i meant "page actions"...)

Comment 4

[jscher2000]

On Windows 7 x64, if I add that Page Actions item to the Address Bar, I do get the current site icon. It does not have a plus and it does not work when I click it.

NS_ERROR_FAILURE: addEngine: Error adding engine:
[Exception... "Engine location is neither a File nor a URI object"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/nsSearchService.js :: ERROR :: line 218"  data: no]

(The menu item works from the menu itself regardless of whether it is added to the Address Bar.)

In various experiments, while I had SuMo and BMO open simultaneously, I once saw the button on the SuMo tab drop a list of both sites, which I don't think should happen if the page action script is scoped to the OpenSearch links in the current page. I haven't seen that again, but taken together with the main error, it suggests some confusion about context with this button.

Comment 5

[Drew Willcoxon :adw]

There are three different things mentioned here:

(1) The button is blank in the address bar, at least for some people
(2) When the button isn't blank, it doesn't have a plus-sign badge
(3) Clicking the button doesn't do anything

(1) has already been filed as bug 1449200.  It's not specific to the add-search-engine button, and I think it must be fallout from Kris's recent work that changed how icons are set.  I can't reproduce the problem on macOS, so I'm wondering if this only happens on Windows and Linux, but that seems weird.

(2) is expected.  The original bug landed without it.  The problem is that badged buttons are more complicated than an <image>, which is what the button is in the urlbar.

(3) is a problem.  I'm going to morph this bug into that.

Comment 6

[Drew Willcoxon :adw]

Attachment: Bug 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar.

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

Comment 7

[Drew Willcoxon :adw]

A couple of problems:

(1) The main problem is that the <image> in the urlbar is expected to have "uri" and "image" attributes that represent the engine, but it doesn't.

(2) A secondary problem is that when an engine button in the subview is clicked, PanelMultiView.hidePopup() is called unconditionally on the main panel, but of course the subview may be in the activated-action panel, not the main panel.

The fixes are simple:

(1) Only use those "uri" and "image" attributes for buttons in the subview.  In the simple case where there's only one engine, just call _installEngine directly.

(2) Call hidePopup() on the closest panel.

Most of this patch is adding tests.

Comment 8

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53b4cfb7ebd8701d30d1315a9257d273d4632a1a

Comment 9

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #5)
> There are three different things mentioned here:
> ...
> (2) When the button isn't blank, it doesn't have a plus-sign badge

I filed bug 1450125 for this.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8963778 [details]
Bug 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar.

https://reviewboard.mozilla.org/r/232646/#review238180

::: browser/base/content/browser-pageActions.js:1230
(Diff revision 1)
>          return;
>        }
>      }
> -    this._handleClickOnEngineButton(buttonNode);
> -  },
> -
> +    // Either the panel button or urlbar button was clicked -- not a button in
> +    // the subview -- but in either case, there's only one search engine.
> +    // (Because this method isn't called when the panel button is clicked and it

I was initially confused by this comment, and upon re-reading a bunch of stuff in the other bug, I now understand better my confusion there, too - you're only calling onCommand for urlbar buttons, but not for items in the panel that have subviews? That seems pretty surprising. Why don't we call onCommand there?

I think we should either call it in all cases, or call it when there's not a subview. Calling it when there's not a subview, except for the url bar where we always call it, is super confusing. It's a command event handler. It should do what it says on the tin.

I don't really understand why the search engine stuff needs to toggle its subview-ness within onCommand - presumably it could just do that whenever it gets told about the search engines? Put differently, the icon information gets updated when switching tabs / loading pages / what-have-you, so presumably we can just set wantssubview from there... Anyway, if it really really really needs a thing, it would be less confusing to have a separately named method that we call for urlbar actions.


Obviously none of this is directly related to this patch, so I'm not asking for anything to change in this bug, but please file a follow-up bug to address this.

::: browser/base/content/browser-pageActions.js:1235
(Diff revision 1)
> -
> -  _handleClickOnEngineButton(button) {
> -    this._installEngine(button.getAttribute("uri"),
> -                        button.getAttribute("image"));
> +    // (Because this method isn't called when the panel button is clicked and it
> +    // shows a subview, and the many-engines case for the urlbar returned early
> +    // above.)
> +    let engine = this.engines[0];
> +    if (!engine) {
> +      // Shouldn't happen...

So why do we nullcheck? :-)

Seems better to let this error if there isn't an engine, then at least we find out about broken assumptions, instead of it just failing silently.

Comment 11

[Drew Willcoxon :adw]

(In reply to :Gijs from comment #10)
> I was initially confused by this comment, and upon re-reading a bunch of
> stuff in the other bug, I now understand better my confusion there, too -
> you're only calling onCommand for urlbar buttons, but not for items in the
> panel that have subviews? That seems pretty surprising. Why don't we call
> onCommand there?

Fair question, but I think it makes sense.  I'll answer this part first:

> I don't really understand why the search engine stuff needs to toggle its
> subview-ness within onCommand - presumably it could just do that whenever it
> gets told about the search engines? Put differently, the icon information
> gets updated when switching tabs / loading pages / what-have-you, so
> presumably we can just set wantssubview from there...

I'm trying to update as little as possible, do as little as possible, every time you switch tabs.  As you say, this action potentially needs to be updated every time you switch tabs.  That's why I made panel initialization lazy.  It's also why I made this change.

In the panel, buttons that trigger subviews are immediately obvious because they show the subviewbutton-nav arrow.  Once the panel is shown, it's not really possible to delay any further deciding whether an action wants a subview.  It would be weird to click an action in the panel that doesn't look like it wants a subview only to have it trigger a subview, or vice versa.

That's not true for actions in the urlbar.  It's possible to completely delay the subview decision right up until you click them.

Aside from the performance aspect, I don't think it makes much sense to call onCommand for subviewbutton-nav buttons in the panel.  At that point, the subviewbutton-nav arrow is showing.  Clicking the button should do only one thing: show the subview.  There's no good reason to call onCommand in that case.

> ::: browser/base/content/browser-pageActions.js:1235
> (Diff revision 1)
> > -
> > -  _handleClickOnEngineButton(button) {
> > -    this._installEngine(button.getAttribute("uri"),
> > -                        button.getAttribute("image"));
> > +    // (Because this method isn't called when the panel button is clicked and it
> > +    // shows a subview, and the many-engines case for the urlbar returned early
> > +    // above.)
> > +    let engine = this.engines[0];
> > +    if (!engine) {
> > +      // Shouldn't happen...
> 
> So why do we nullcheck? :-)
> 
> Seems better to let this error if there isn't an engine, then at least we
> find out about broken assumptions, instead of it just failing silently.

OK

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8963778 [details]
Bug 1449947 - The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232646/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a86e795540dc
The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar. r=Gijs

Comment 14

[Narcis Beleuzu [:NarcisB]]

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

Comment 15

[Sajedul Islam]

I have reproduced this bug with Nightly 61.0a1 (2018-03-29) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly!

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

[bugday-20180404]

Comment 16

[Bogdan Maris, Desktop QA]

Thanks Sajedul Islam for verifying this!