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

VERIFIED FIXED in Firefox 61

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: johnmaverick74, Assigned: adw)

Tracking

61 Branch
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

Reporter

Description

a year ago
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

a year ago
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?
Blocks: 1221539
Flags: needinfo?(johnmaverick74)
Flags: needinfo?(adw)
Reporter

Comment 2

a year ago
(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
Flags: needinfo?(johnmaverick74)
Reporter

Comment 3

a year ago
(i meant "page actions"...)

Comment 4

a year ago
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.
Assignee

Comment 5

a year ago
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.
Flags: needinfo?(adw)
Assignee

Updated

a year ago
Status: UNCONFIRMED → NEW
Component: Untriaged → Address Bar
Ever confirmed: true
Summary: Adding the "New Search Engine" Button to Address Bar results in a blank button → The "Add Search Engine" page action button doesn't responds to clicks when it's in the urlbar/address bar/location bar
Assignee

Updated

a year ago
Assignee: nobody → adw
Priority: -- → P1
See Also: → 1449200
Whiteboard: [fxsearch]
Assignee

Updated

a year ago
Summary: The "Add Search Engine" page action button doesn't responds to clicks when it's in the urlbar/address bar/location bar → The "Add Search Engine" page action button doesn't respond to clicks when it's in the urlbar/address bar/location bar
Assignee

Comment 7

a year ago
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.
Assignee

Updated

a year ago
Status: NEW → ASSIGNED
Assignee

Comment 9

a year ago
(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.
See Also: → 1450125

Comment 10

a year ago
mozreview-review
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.
Attachment #8963778 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 11

a year ago
(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 hidden (mozreview-request)

Comment 13

a year ago
Pushed by dwillcoxon@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a86e795540dc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Comment 15

a year ago
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]
Thanks Sajedul Islam for verifying this!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.