Closed Bug 1221539 Opened 9 years ago Closed 7 years ago

Add search engines discovery to the location bar

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
relnote-firefox --- 61+
firefox61 --- verified

People

(Reporter: mak, Assigned: adw)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [consistency][fxsearch])

Attachments

(5 files)

idea of adding a search engine
5.68 KB, image/png
Details
[SPEC] Add One-Click Search Engines.pdf
1.43 MB, application/pdf
Details
Example of menu with extension actions
89.33 KB, image/png
Details
Bug 1221539 - Add search engine discovery to the page action menu. Part 1: Page action changes.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1221539 - Add search engine discovery to the page action menu. Part 2: Add the new action.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
      No description provided.
To be consistent across different search points, we likely want to be able to add a search engine from the location bar.
Very likely this should be conditioned by the fact the search bar has been removed or not, cause I suppose we don't want multiple visual notifications for the same event.
Priority: -- → P2
Summary: Implement search engines discovery in the location bar → Add search engines discovery to the location bar
Blocks: 1212397
This needs UX
Keywords: uiwanted
Whiteboard: [consistency][fxsearch]
Priority: P2 → P3
Priority: P3 → P2
This is planned for 58 or later.
> This is planned for 58 or later.

That's a pretty serious problem if we are going to remove the search bar in 57.
sorry for the duplicate (i did search, but not for the right words, apparently)...

About the feature: it would be great to see the search bar finally go away. To me it's probably the most useless item in FF UI. The only reason for it to be there is to add new search engines. The first UI i've seen from Nightly, without the search bar, was great! The last (with it) is more like "meh..."

Question really is: with the amount of work "you" developers sure have, how much time consuming would this be?
(In reply to johnmaverick74 from comment #9)
> sorry for the duplicate (i did search, but not for the right words,
> apparently)...
> 
> About the feature: it would be great to see the search bar finally go away.
> To me it's probably the most useless item in FF UI. The only reason for it
> to be there is to add new search engines. The first UI i've seen from
> Nightly, without the search bar, was great! The last (with it) is more like
> "meh..."
> 
> Question really is: with the amount of work "you" developers sure have, how
> much time consuming would this be?

Hey John, I'm thinking along the same lines as you.  I've done some UX explorations for this and it's proving tricky to implement without adding clutter and noise to the address bar.  I'm having trouble justifying if this feature should hold the same weight as other items in the address bar such as the privacy lock.  I hoping to get more data on this to see if this is something how many users use this feature.
(In reply to Eric Pang [:epang] UX from comment #10)
> (In reply to johnmaverick74 from comment #9)
> > sorry for the duplicate (i did search, but not for the right words,
> > apparently)...
> > 
> > About the feature: it would be great to see the search bar finally go away.
> > To me it's probably the most useless item in FF UI. The only reason for it
> > to be there is to add new search engines. The first UI i've seen from
> > Nightly, without the search bar, was great! The last (with it) is more like
> > "meh..."
> > 
> > Question really is: with the amount of work "you" developers sure have, how
> > much time consuming would this be?
> 
> Hey John, I'm thinking along the same lines as you.  I've done some UX
> explorations for this and it's proving tricky to implement without adding
> clutter and noise to the address bar.  I'm having trouble justifying if this
> feature should hold the same weight as other items in the address bar such
> as the privacy lock.  I hoping to get more data on this to see if this is
> something how many users use this feature.

How about adding an entry to the three dot (...) button? 
ok, it will loose some visibility when a new search engine is present, but it would not clutter the UI that much. 
Another option would be the "Search Engine Icon" with a "plus sign" right beside the search settings button on the location bar dropdown...
Button to add a search engine from the awesome bar
(In reply to johnmaverick74 from comment #11)
> (In reply to Eric Pang [:epang] UX from comment #10)
> > (In reply to johnmaverick74 from comment #9)
> > > sorry for the duplicate (i did search, but not for the right words,
> > > apparently)...
> > > 
> > > About the feature: it would be great to see the search bar finally go away.
> > > To me it's probably the most useless item in FF UI. The only reason for it
> > > to be there is to add new search engines. The first UI i've seen from
> > > Nightly, without the search bar, was great! The last (with it) is more like
> > > "meh..."
> > > 
> > > Question really is: with the amount of work "you" developers sure have, how
> > > much time consuming would this be?
> > 
> > Hey John, I'm thinking along the same lines as you.  I've done some UX
> > explorations for this and it's proving tricky to implement without adding
> > clutter and noise to the address bar.  I'm having trouble justifying if this
> > feature should hold the same weight as other items in the address bar such
> > as the privacy lock.  I hoping to get more data on this to see if this is
> > something how many users use this feature.
> 
> How about adding an entry to the three dot (...) button? 
> ok, it will loose some visibility when a new search engine is present, but
> it would not clutter the UI that much. 

Thanks John, I was thinking the same thing of having it in the page action menu (...)
I'm working with Michelle Heubusch for the copy since it's not as clear what it does when it loses the context of the other One-Click search engines in the search bar. You're right about the lowered visibility, but I'm not too concerned at this point since we don't have the data for this feature.  If in the future it proves to be of high use we can revise and give it more prominence.

> Another option would be the "Search Engine Icon" with a "plus sign" right
> beside the search settings button on the location bar dropdown...

I think this is a great idea for a secondary location to add the one-click search.  I say secondary since the user needs to navigate to the site then re-open the dropdown to see that it's available to be added.  So it's more hidden, but saying that it doesn't hurt to have it in the footer.  I'll work on some mock up for this as well.
Hi Eric,

Thanks for the reply!

Another possibility (that might even get along with the mobile version) would be an "add this search engine" entry on the context menu when one "right-clicks" on the search engine's search field (again, more or less like what we do to add a search engine on Firefox Mobile...)

We already have an "Add a keyword for this search" on the context menu but, honestly, i always found that very confusing!!! 

I never know if it will add a keyword, a search engine or a bookmark... It has a very weird behavior (or maybe it's just me...)

i wish it would be just an "add this search engine"...
(In reply to Eric Pang [:epang] UX from comment #13)
> (In reply to johnmaverick74 from comment #11)
> > (In reply to Eric Pang [:epang] UX from comment #10)
> > > (In reply to johnmaverick74 from comment #9)
> > > > sorry for the duplicate (i did search, but not for the right words,
> > > > apparently)...
> > > > 
> > > > About the feature: it would be great to see the search bar finally go away.
> > > > To me it's probably the most useless item in FF UI. The only reason for it
> > > > to be there is to add new search engines. The first UI i've seen from
> > > > Nightly, without the search bar, was great! The last (with it) is more like
> > > > "meh..."
> > > > 
> > > > Question really is: with the amount of work "you" developers sure have, how
> > > > much time consuming would this be?
> > > 
> > > Hey John, I'm thinking along the same lines as you.  I've done some UX
> > > explorations for this and it's proving tricky to implement without adding
> > > clutter and noise to the address bar.  I'm having trouble justifying if this
> > > feature should hold the same weight as other items in the address bar such
> > > as the privacy lock.  I hoping to get more data on this to see if this is
> > > something how many users use this feature.
> > 
> > How about adding an entry to the three dot (...) button? 
> > ok, it will loose some visibility when a new search engine is present, but
> > it would not clutter the UI that much. 
> 
> Thanks John, I was thinking the same thing of having it in the page action
> menu (...)
> I'm working with Michelle Heubusch for the copy since it's not as clear what
> it does when it loses the context of the other One-Click search engines in
> the search bar. You're right about the lowered visibility, but I'm not too
> concerned at this point since we don't have the data for this feature.  If
> in the future it proves to be of high use we can revise and give it more
> prominence.
> 
> > Another option would be the "Search Engine Icon" with a "plus sign" right
> > beside the search settings button on the location bar dropdown...
> 
> I think this is a great idea for a secondary location to add the one-click
> search.  I say secondary since the user needs to navigate to the site then
> re-open the dropdown to see that it's available to be added.  So it's more
> hidden, but saying that it doesn't hurt to have it in the footer.  I'll work
> on some mock up for this as well.

Sorry, forgot to make "reply"... reply above :)
(In reply to Eric Pang [:epang] UX from comment #13)
> (In reply to johnmaverick74 from comment #11)
> > (In reply to Eric Pang [:epang] UX from comment #10)
> > How about adding an entry to the three dot (...) button? 
> > ok, it will loose some visibility when a new search engine is present, but
> > it would not clutter the UI that much. 
> 
> Thanks John, I was thinking the same thing of having it in the page action
> menu (...)
> I'm working with Michelle Heubusch for the copy since it's not as clear what
> it does when it loses the context of the other One-Click search engines in
> the search bar. You're right about the lowered visibility, but I'm not too
> concerned at this point since we don't have the data for this feature.  If
> in the future it proves to be of high use we can revise and give it more
> prominence.

But how will high use ever possibly occur if the typical user is unaware of the option? If we want to aid **discoverability** then I think it would be preferable to default to visible, for example:

Have the + icon appear next to the page actions menu (e.g., with Pocket and Bookmark) when (1) the site has a search engine available AND (2) there is no existing search engine with the identical name AND (3) the search bar is not displayed on the toolbar AND (4) the user has not previously removed the button from the address bar. 

If the user does not find this feature of value, she could right-click > Remove from Address Bar as with other functions on the page actions menu. Then the + action would be enabled on the page actions menu when conditions (1) and (2) are fulfilled.
I must say i like a lot of @jscher2000 's solution!!! =)
(In reply to jscher2000 from comment #16)
> (In reply to Eric Pang [:epang] UX from comment #13)
> > (In reply to johnmaverick74 from comment #11)
> > > (In reply to Eric Pang [:epang] UX from comment #10)
> > > How about adding an entry to the three dot (...) button? 
> > > ok, it will loose some visibility when a new search engine is present, but
> > > it would not clutter the UI that much. 
> > 
> > Thanks John, I was thinking the same thing of having it in the page action
> > menu (...)
> > I'm working with Michelle Heubusch for the copy since it's not as clear what
> > it does when it loses the context of the other One-Click search engines in
> > the search bar. You're right about the lowered visibility, but I'm not too
> > concerned at this point since we don't have the data for this feature.  If
> > in the future it proves to be of high use we can revise and give it more
> > prominence.
> 
> But how will high use ever possibly occur if the typical user is unaware of
> the option? If we want to aid **discoverability** then I think it would be
> preferable to default to visible, for example:
> 
> Have the + icon appear next to the page actions menu (e.g., with Pocket and
> Bookmark) when (1) the site has a search engine available AND (2) there is
> no existing search engine with the identical name AND (3) the search bar is
> not displayed on the toolbar AND (4) the user has not previously removed the
> button from the address bar. 
> 
> If the user does not find this feature of value, she could right-click >
> Remove from Address Bar as with other functions on the page actions menu.
> Then the + action would be enabled on the page actions menu when conditions
> (1) and (2) are fulfilled.

Hi @jscher2000, I'm working on something similar to your suggestion!  I'm also really concerned about discoverability.  The difference is that I'm planning to have the icon behave in the same way as reader mode.  Functionality wise this makes sense since both are actions that appear under the right conditions.  I'm currently exploring the ux and motion to help clarify what this feature does to the user.
(In reply to Eric Pang [:epang] UX from comment #18)
> (In reply to jscher2000 from comment #16)
> > (In reply to Eric Pang [:epang] UX from comment #13)
> > > (In reply to johnmaverick74 from comment #11)
> > > > (In reply to Eric Pang [:epang] UX from comment #10)
> > > > How about adding an entry to the three dot (...) button? 
> > > > ok, it will loose some visibility when a new search engine is present, but
> > > > it would not clutter the UI that much. 
> > > 
> > > Thanks John, I was thinking the same thing of having it in the page action
> > > menu (...)
> > > I'm working with Michelle Heubusch for the copy since it's not as clear what
> > > it does when it loses the context of the other One-Click search engines in
> > > the search bar. You're right about the lowered visibility, but I'm not too
> > > concerned at this point since we don't have the data for this feature.  If
> > > in the future it proves to be of high use we can revise and give it more
> > > prominence.
> > 
> > But how will high use ever possibly occur if the typical user is unaware of
> > the option? If we want to aid **discoverability** then I think it would be
> > preferable to default to visible, for example:
> > 
> > Have the + icon appear next to the page actions menu (e.g., with Pocket and
> > Bookmark) when (1) the site has a search engine available AND (2) there is
> > no existing search engine with the identical name AND (3) the search bar is
> > not displayed on the toolbar AND (4) the user has not previously removed the
> > button from the address bar. 
> > 
> > If the user does not find this feature of value, she could right-click >
> > Remove from Address Bar as with other functions on the page actions menu.
> > Then the + action would be enabled on the page actions menu when conditions
> > (1) and (2) are fulfilled.
> 
> Hi @jscher2000, I'm working on something similar to your suggestion!  I'm
> also really concerned about discoverability.  The difference is that I'm
> planning to have the icon behave in the same way as reader mode. 
> Functionality wise this makes sense since both are actions that appear under
> the right conditions.  I'm currently exploring the ux and motion to help
> clarify what this feature does to the user.

@Eric, is there any hope to see this shipping in FF57?
(In reply to johnmaverick74 from comment #19)
> @Eric, is there any hope to see this shipping in FF57?

Unlikely. 57 is pretty much closed, only critical changes affecting security or reliability can land.
Hi Marco,

Sorry for the delay on this!

After going back and forth and discussing this with the UX team we decided to add this feature into the page action menu (and the search drop down as a secondary location).

I explored some more discoverable locations but the current state of the feature doesn't warrant high visibility.  In the future we plan to explore the concept of 1-click search engines more.  At that time with more research and exploration we can make the feature more visible.

I've attached the spec, please NI or ping me if you have any questions!
Thanks!
Flags: needinfo?(mak77)
The only thing still under-specified, is what happens if the user "Add to the Address Bar" this entry. Should that be prevented?
Flags: needinfo?(mak77) → needinfo?(epang)
(In reply to Marco Bonardo [::mak] from comment #24)
> The only thing still under-specified, is what happens if the user "Add to
> the Address Bar" this entry. Should that be prevented?

For the time being let's prevent this feature from being added to the address bar.  Thanks!
Flags: needinfo?(epang)
(In reply to Eric Pang [:epang] UX from comment #23)

> After going back and forth and discussing this with the UX team we decided
> to add this feature into the page action menu (and the search drop down as a
> secondary location).
> 
> I explored some more discoverable locations but the current state of the
> feature doesn't warrant high visibility.

Hi Eric. I like the idea of making this available in the page action menu, but a large part of what we want in this bug is to make open search plugins discoverable, and this doesn't seem to be addressed by the attached spec. In the searchbar there was a green (+) badge above the glass icon when an open search plugin was offered by the page. Could we add something similar above the dots of the page action icon? Or maybe put a small glass icon above the dots instead of the (+) badge?

I'm afraid implementing the UI proposed in the current spec is a lot of work for something very few users will ever notice if we don't make it more discoverable.
Flags: needinfo?(epang)
Opinion:

I understand the concerns of discoverability and i also agree there should be some indicator that a search engine is available to be added. 

However, i believe it is a feature not much used and even if it is, if a user wants to add it, he just need to go to the page actions and see if the plugin is available.

I also believe that, for now, having this implementation is better than having none, so I'm all in favor of landing this asap and then improve it in the near future.

(By the way I'm also concern with: everytime a user visits a page that has a search plugin available, one will have a "new icon" hanging on the address bar. It may became annoying and create clutter... or not... Anyway, I can't think of any other way to do it atm...)
(In reply to Eric Pang [:epang] UX from comment #25)
> (In reply to Marco Bonardo [::mak] from comment #24)
> > The only thing still under-specified, is what happens if the user "Add to
> > the Address Bar" this entry. Should that be prevented?
> 
> For the time being let's prevent this feature from being added to the
> address bar.  Thanks!

It seems that every other item on the Page Actions menu allows right-click > Add to Address bar. Why exclude this one? Even if few would use it, I think it would be an unnecessary inconsistency in the UI.

Also, I hope the icon later can be decorated with a "+" and shown/hidden selectively in the address bar when it has a useful function. Preventing the icon from being shown in the address bar would have to be reversed at that point.
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> (In reply to Eric Pang [:epang] UX from comment #23)
> 
> > After going back and forth and discussing this with the UX team we decided
> > to add this feature into the page action menu (and the search drop down as a
> > secondary location).
> > 
> > I explored some more discoverable locations but the current state of the
> > feature doesn't warrant high visibility.
> 
> Hi Eric. I like the idea of making this available in the page action menu,
> but a large part of what we want in this bug is to make open search plugins
> discoverable, and this doesn't seem to be addressed by the attached spec. In
> the searchbar there was a green (+) badge above the glass icon when an open
> search plugin was offered by the page. Could we add something similar above
> the dots of the page action icon? Or maybe put a small glass icon above the
> dots instead of the (+) badge?
> 
> I'm afraid implementing the UI proposed in the current spec is a lot of work
> for something very few users will ever notice if we don't make it more
> discoverable.

Hi Florian, I completely agree and understand your concerns.  But for now it's intentional that we are keeping it more discrete.
As John mentioned it doesn't make sense for a niche feature to have such high visual prominence (at least not the way it functions now). For now it's about giving users a way to still use the feature.  The plan is to revisit this in the near future to make it more useful to the user - when it's something we are proud to ship we can give it more visibility.

In regards to jscher2000 comment.  If it's not too much extra work we can add in the right-click > Add to Address bar.  I don't know how much it will be used so I'm not sure if it's worth the engineering effort. But I like that it adds discoverbility for users who want it. Once added, the icon (with '+') should only appear in the URL bar when there is a One-Click Search Engine available to be added.

Thanks!
Flags: needinfo?(epang)
> In regards to jscher2000 comment.  If it's not too much extra work we can
> add in the right-click > Add to Address bar.  I don't know how much it will
> be used so I'm not sure if it's worth the engineering effort. But I like
> that it adds discoverbility for users who want it. Once added, the icon
> (with '+') should only appear in the URL bar when there is a One-Click
> Search Engine available to be added.


I think this would be Gold!!! I also - really - want it this way, but I would hate if the implementation would mean delaying the feature for another version (having to wait for FF59)!
Eric, when there's any new development or eta, please drop a line here.

I'm sorry for asking but i'm very curious to test this feature! Tks
(In reply to Eric Pang [:epang] UX from comment #29)

> In regards to jscher2000 comment.  If it's not too much extra work we can
> add in the right-click > Add to Address bar.  I don't know how much it will
> be used so I'm not sure if it's worth the engineering effort. But I like
> that it adds discoverbility for users who want it. Once added, the icon
> (with '+') should only appear in the URL bar when there is a One-Click
> Search Engine available to be added.


How is this coming along Eric?

Will we get it in a next 58 beta? Or will it be postponed?
(In reply to johnmaverick74 from comment #33)
> (In reply to Eric Pang [:epang] UX from comment #29)
> 
> > In regards to jscher2000 comment.  If it's not too much extra work we can
> > add in the right-click > Add to Address bar.  I don't know how much it will
> > be used so I'm not sure if it's worth the engineering effort. But I like
> > that it adds discoverbility for users who want it. Once added, the icon
> > (with '+') should only appear in the URL bar when there is a One-Click
> > Search Engine available to be added.
> 
> 
> How is this coming along Eric?
> 
> Will we get it in a next 58 beta? Or will it be postponed?

Panos, wondering if you can shed more light here.  Do we have an eta on when we'll have someone who can work on this feature?
Thanks!
Flags: needinfo?(past)
The plan for this is sometime in Q1, so probably Firefox 60 or 61.
Flags: needinfo?(past)
I suppose we're talking about jscher2000's suggestion (on comment 28).
If so, and if Eric's idea is of more quick implementation, i would like to suggest we go with that implementation (Eric's original plan of having the add search engine on the page action's button) ASAP as a means for users to have a "temporary way out" and then on 60 / 61 complete the feature as planned.

Even without  enhanced features is better that nothing for now...

It really is boring to have to go to options > Search options > Activate search bar > go to page > Add the pretended search engine > go to options again > Search options > and deactivate the search bar again, just to add a new search engine... which is that "thing"'s only function now.
Priority: P2 → P1
Eric, we already add extension page actions to the bottom of the menu, with a separator between them and the built-in actions.  See the attached screenshot for an example.  How/where should the add-engine item appear in relation to them?

IMO there are two options, either between the built-in actions and extension actions, with a separator as necessary; or at the very bottom even when there are extension actions, also with a separator.
Flags: needinfo?(epang)
(In reply to Drew Willcoxon :adw from comment #41)
> Created attachment 8948833 [details]
> Example of menu with extension actions
> 
> Eric, we already add extension page actions to the bottom of the menu, with
> a separator between them and the built-in actions.  See the attached
> screenshot for an example.  How/where should the add-engine item appear in
> relation to them?
> 
> IMO there are two options, either between the built-in actions and extension
> actions, with a separator as necessary; or at the very bottom even when
> there are extension actions, also with a separator.

Hi Drew, let's put it at the very bottom with a separator.  This will keep location consistency for any users that use this functionality.  Thanks for the NI and working on this!
Flags: needinfo?(epang)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Early WIP
More WIP.  I'm modifying PageActions to take into account a couple of new concepts: (1) An action may want to change whether or not it has a subview.  The add-engine action needs to do that because one page may have one engine but another page may have many engines.  (2) An action may want to stick itself to the bottom of the panel, and also hide itself when it's disabled instead of being visible but disabled.

So it's probably a good idea to split this into two different patches, one doing the things just mentioned, and another adding the new add-engine action.
Basically working, need to clean up my mess, fix tests, and split the patch into two for review.
@Drew 

Is there still time to get it into FF59? Or will we have to wait to FF60?
Feature- and test-complete patch, now need to clean it up and split it up for review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc6ba873af67859398caaca241c7ebddd2f9ca5
I'm out tomorrow and only working part of the day on Monday, so you may be better off asking e.g. Florian or Jared for a review. Otherwise I will do my best to review this Monday, maybe Tuesday. Sorry!
OK, thanks Gijs.  I do think you would be the best person to look at part 1 at least.

I'll describe the changes for whoever ends up reviewing.

Part 1 is foundational page actions changes/improvements.  The main changes are:

* Transient actions: I invented a new category just for this new add-engine item.  These are actions that stick to the bottom of the panel.  Unlike normal actions, when they're disabled, they disappear from the panel.

* Change action.subview to action.setWantsSubview()/getWantsSubview().  The add-engine action needs to be able to switch between wanting a subview (when there are multiple engines offered) and not (when there's a single engine).  And it needs to do it per window.

* For urlbar buttons, call action.onCommand() even if the action has a subview (before showing the subview panel) so that the action can call setWantsSubview() if it wants.

Other changes that kind of crept in and aren't strictly necessary but are nice:

* Place actions in the panel lazily, when it's opened
* Remove PageActions.Subview, PageActions.Button -- the only thing using these is the send-to-device action, and it barely uses them.  Don't need them.

Part 2 adds the new add-engine action.  It's straightforward compared to part 1.  It moves some code out of search.xml that's used to update the search bar's add-engine badge, into browser.js's BrowserSearch, so that both the search bar and this new action can rely on it.
Fixes for webcompat and screenshots TV --verify tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3695541c23da1ada58bd2062c1139eae19ce426c
Bug 1446250 bitrotted this and I had no idea about it, nice....
Comment on attachment 8961608 [details]
Bug 1221539 - Add search engine discovery to the page action menu. Part 2: Add the new action.

https://reviewboard.mozilla.org/r/230472/#review236634

When I test this on ecosia.org, when adding the search engine the error "Invalid string label" is logged to the browser console. I thought it might be an issue with the opensearch markup used by that engine, but I see the same when trying with qwant (qwant.com) . There's no stack or even a source line so I'm not sure how to figure out what's causing it, but hopefully you can have a look.

Otherwise, the below are mostly just nits and this would have been r+ except for the above, which makes me a bit nervous about this patch.

::: browser/base/content/browser.js:3795
(Diff revision 5)
> +    switch (data) {
> +    case "engine-removed":
> +      // An engine was removed from the search service.  If a page is offering
> +      // the engine, then the engine needs to be added back to the corresponding
> +      // browser's offered engines.
> +      this._addMaybeOfferedEngine(engine);

The only thing the add/hide methods use off the `engine` object is the name, and the only observer notifications this object gets pass an engine (even encoded in the argument names of the method!).

Can we just extract the name inside `observe()` and pass the engine name to the `_add` and `_hideMaybeOfferedEngine` methods?

::: browser/base/content/browser.js:3829
(Diff revision 5)
> +    if (selectedBrowserOffersEngine) {
> +      this.updateOpenSearchBadge();
> +    }
> +  },
> +
> +  _hideMaybeOfferedEngine(engine) {

Please make the method names match - either `show/hide` or `add/remove` not `add/hide` which is confusing.

::: browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js:4
(Diff revision 5)
> +  // Disable panel animations.  They cause intermittent timeouts on Linux when
> +  // the test tries to synthesize clicks on items in newly opened panels.
> +  BrowserPageActions._disablePanelAnimations = true;
> +  registerCleanupFunction(() => {
> +    BrowserPageActions._disablePanelAnimations = false;
> +  });

Is this copied from elsewhere? My understanding is that after Paolo's changes to panelmultiview, this shouldn't be necessary anymore. If you haven't, can you check if this is really necessary? And if you have and it's necessary, can you file a follow-up bug to get this fixed?

::: browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js:24
(Diff revision 5)
> +    EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
> +    await promisePageActionPanelHidden();
> +
> +    // The action should not be present.
> +    let actions = PageActions.actionsInPanel(window);
> +    let action = actions.find(a => a.id == "addSearchEngine");

Could just use `.some`

::: browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js:28
(Diff revision 5)
> +    let actions = PageActions.actionsInPanel(window);
> +    let action = actions.find(a => a.id == "addSearchEngine");
> +    Assert.ok(!action, "Action should not be present in panel");
> +    let button =
> +      BrowserPageActions.panelButtonNodeForActionID("addSearchEngine");
> +    Assert.ok(!button);

Nit: please add a message, here and below.

::: browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js:35
(Diff revision 5)
> +});
> +
> +
> +// Checks a page that offers one engine.
> +add_task(async function one() {
> +  let url = "http://mochi.test:8888/browser/browser/base/content/test/urlbar/page_action_menu_add_search_engine_one.html";

Here and below, can you use `gTestPath` and friends to avoid hardcoding the entire path?

::: browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js:291
(Diff revision 5)
> +  return new Promise(resolve => {
> +    let topic = "browser-search-engine-modified";
> +    Services.obs.addObserver(function obs(engine, t, data) {
> +      if ("engine-added" == data &&
> +          engine.wrappedJSObject.name == expectedName) {
> +        Services.obs.removeObserver(obs, topic);
> +        resolve(engine);
> +      }
> +    }, topic);
> +  });

Nit: TestUtils.topicObserved() should make this simpler.

::: browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.js:304
(Diff revision 5)
> +    }, topic);
> +  });
> +}
> +
> +function promiseEngineRemoved(expectedName) {
> +  return new Promise(resolve => {

Ditto
Attachment #8961608 - Flags: review?(gijskruitbosch+bugs)
Sorry, still trying to get to grips with the first cset... Going to be tomorrow before I'll be able to finish reviewing it.
(In reply to :Gijs from comment #84)
> When I test this on ecosia.org, when adding the search engine the error
> "Invalid string label" is logged to the browser console.

It's because of telemetry: I added a new built-in action, and each built-in action has a label in the page-actions histograms, but I didn't add a label for this new action.

> ::: browser/base/content/browser.js:3795
> (Diff revision 5)
> > +    switch (data) {
> > +    case "engine-removed":
> > +      // An engine was removed from the search service.  If a page is offering
> > +      // the engine, then the engine needs to be added back to the corresponding
> > +      // browser's offered engines.
> > +      this._addMaybeOfferedEngine(engine);
> 
> The only thing the add/hide methods use off the `engine` object is the name,
> and the only observer notifications this object gets pass an engine (even
> encoded in the argument names of the method!).
> 
> Can we just extract the name inside `observe()` and pass the engine name to
> the `_add` and `_hideMaybeOfferedEngine` methods?

Done

> ::: browser/base/content/browser.js:3829
> (Diff revision 5)
> > +    if (selectedBrowserOffersEngine) {
> > +      this.updateOpenSearchBadge();
> > +    }
> > +  },
> > +
> > +  _hideMaybeOfferedEngine(engine) {
> 
> Please make the method names match - either `show/hide` or `add/remove` not
> `add/hide` which is confusing.

Done

> browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.
> js:4
> (Diff revision 5)
> > +  // Disable panel animations.  They cause intermittent timeouts on Linux when
> > +  // the test tries to synthesize clicks on items in newly opened panels.
> > +  BrowserPageActions._disablePanelAnimations = true;
> > +  registerCleanupFunction(() => {
> > +    BrowserPageActions._disablePanelAnimations = false;
> > +  });
> 
> Is this copied from elsewhere? My understanding is that after Paolo's
> changes to panelmultiview, this shouldn't be necessary anymore. If you
> haven't, can you check if this is really necessary? And if you have and it's
> necessary, can you file a follow-up bug to get this fixed?

Removed

> browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.
> js:24
> (Diff revision 5)
> > +    EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
> > +    await promisePageActionPanelHidden();
> > +
> > +    // The action should not be present.
> > +    let actions = PageActions.actionsInPanel(window);
> > +    let action = actions.find(a => a.id == "addSearchEngine");
> 
> Could just use `.some`

Done

> browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.
> js:28
> (Diff revision 5)
> > +    let actions = PageActions.actionsInPanel(window);
> > +    let action = actions.find(a => a.id == "addSearchEngine");
> > +    Assert.ok(!action, "Action should not be present in panel");
> > +    let button =
> > +      BrowserPageActions.panelButtonNodeForActionID("addSearchEngine");
> > +    Assert.ok(!button);
> 
> Nit: please add a message, here and below.

Done

> browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.
> js:35
> (Diff revision 5)
> > +});
> > +
> > +
> > +// Checks a page that offers one engine.
> > +add_task(async function one() {
> > +  let url = "http://mochi.test:8888/browser/browser/base/content/test/urlbar/page_action_menu_add_search_engine_one.html";
> 
> Here and below, can you use `gTestPath` and friends to avoid hardcoding the
> entire path?

Done

> browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.
> js:291
> (Diff revision 5)
> > +  return new Promise(resolve => {
> > +    let topic = "browser-search-engine-modified";
> > +    Services.obs.addObserver(function obs(engine, t, data) {
> > +      if ("engine-added" == data &&
> > +          engine.wrappedJSObject.name == expectedName) {
> > +        Services.obs.removeObserver(obs, topic);
> > +        resolve(engine);
> > +      }
> > +    }, topic);
> > +  });
> 
> Nit: TestUtils.topicObserved() should make this simpler.

Done

> browser/base/content/test/urlbar/browser_page_action_menu_add_search_engine.
> js:304
> (Diff revision 5)
> > +    }, topic);
> > +  });
> > +}
> > +
> > +function promiseEngineRemoved(expectedName) {
> > +  return new Promise(resolve => {
> 
> Ditto

Done
Comment on attachment 8949631 [details]
Bug 1221539 - Add search engine discovery to the page action menu. Part 1: Page action changes.

https://reviewboard.mozilla.org/r/218982/#review237012

::: browser/base/content/browser-pageActions.js:52
(Diff revision 13)
>     * Inits.  Call to init.
>     */
>    init() {
>      this.placeAllActions();
> +    this.panelNode.addEventListener("popupshowing", () => {
> +      this.mainButtonNode.setAttribute("open", "true");

Doing this immediately when the item is clicked helps with not having the button state flicker from :hover-> :active -> :hover -> [open] . Can we continue doing that?

::: browser/base/content/browser-pageActions.js:53
(Diff revision 13)
>     */
>    init() {
>      this.placeAllActions();
> +    this.panelNode.addEventListener("popupshowing", () => {
> +      this.mainButtonNode.setAttribute("open", "true");
> +      this._onPanelShowing();

If we move the `open` attribute setting, then in init you can do something like:

```js
this._onPanelShowing = this._onPanelShowing.bind(this);
this.panelNode.addEventListener("popupshowing", this._onPanelShowing);
```

Rather than having some code directly in the event listener and some code in the `_onPanelShowing` method that then doesn't have any other callers.

::: browser/base/content/browser-pageActions.js:68
(Diff revision 13)
> +      let buttonNode = this.panelButtonNodeForActionID(action.id);
> +      action.onShowingInPanel(buttonNode);
> +    }
>    },
>  
> +  _placeLazyActionsInPanelNow() {

As noted in UITour, I think this should probably be public if we call it from outside here. I also think that the `Now` bit doesn't really add anything - you expect a method called `doThing` to do the thing now, unless it explicitly says `doThingAsync` or `doThingLazy` or whatever.

::: browser/base/content/browser-pageActions.js:89
(Diff revision 13)
> -    // Place actions in the urlbar.  Do this in reverse order.  The reason is
> +    for (let action of urlbarActions) {
> -    // subtle.  If there were no urlbar nodes already in markup (like the
> -    // bookmark star button), then doing this in forward order would be fine.
> -    // Forward order means that the insert-before relationship is always broken:
> -    // there's never a next-sibling node before which to insert a new node, so
> -    // node.insertBefore() is always passed null, and nodes are always appended.
> -    // That will break the position of nodes that should be inserted before
> -    // nodes that are in markup, which in turn can break other nodes.
> -    let actionsInUrlbar = PageActions.actionsInUrlbar(window);

Why do we no longer need to do this in reverse order as this comment indicates? I still see the star-button-box in browser.xul in DXR...

::: browser/base/content/browser-pageActions.js:112
(Diff revision 13)
>     *
>     * @param  action (PageActions.Action, required)
>     *         The action to place.
>     */
>    placeActionInPanel(action) {
> +    if (this.panelNode.state == "open") {

Maybe it'd be safer to have this be `!= "closed"` so that we also do this if someone adds an action from a popupshowing handler or similar (when state == "showing") ?

::: browser/base/content/browser-pageActions.js:205
(Diff revision 13)
> +   *         return the panel button ID.  If you're inserting into the urlbar,
> +   *         then it should return the urlbar button ID.
> +   * @return (DOM node, maybe null) The DOM node before which to insert the
> +   *         given action.  Null if the action should be inserted at the end.
> +   */
> +  _insertBeforeNode(action, actionArray, nodeGetterFn) {

This name implies we're inserting the node - can we name it `getNextNode` or something?

Also, the node getter function seems overkill - can we just pass a bool or string that indicates whether this is a panel/urlbar node we're looking for? Then push the actual fetching of the id into this method. This would also mean we can push the actionArray getting into this method, because both callsites just fetch that list and then don't do anything with it.

::: browser/base/content/browser-pageActions.js:659
(Diff revision 13)
> +      if (panelViewNode) {
> +        panelViewNode.remove();
> +      }
> +      return;
> -      }
> +    }
> -      node.setAttribute("tooltiptext", tooltip || title);
> +    panelNode.classList.add("subviewbutton-nav");

Move this above the `if (!wantsSubview)` block and just use:

```js
panelNode.classList.toggle("subviewbutton-nav", wantsSubview);
```

(and remove the identical call inside the if block)

::: browser/base/content/browser-pageActions.js:688
(Diff revision 13)
> -    if (action.subview || action.wantsIframe) {
> +    if (!aaPanelNode || aaPanelNode.getAttribute("actionID") != action.id) {
> +      action.onCommand(event, buttonNode);
> +    }

I'm very confused - how does `onSubviewShowing` still get called if the action's `onCommand` changes its `wantsSubview` property? It looks like that's supposed to happen above this code, and the `togglePanelForAction` code will cause the panel to close and then reopen showing only the relevant subview. I don't think that's what we want.

::: browser/components/uitour/UITour.jsm:161
(Diff revision 13)
>          // It would be hidden if toggled off from the urlbar.
>          let node = aDocument.getElementById("pocket-button-box");
>          if (node && !node.hidden) {
>            return node;
>          }
> +        aDocument.ownerGlobal.BrowserPageActions._placeLazyActionsInPanelNow();

This feels a bit like a code smell... At a minimum, it seems like this should be a public method, given it's called from other modules.

::: browser/components/uitour/UITour.jsm:255
(Diff revision 13)
>                 aDocument.getElementById("pageAction-panel-sendToDevice");
>        },
>      }],
>      ["screenshots", {
>        query: (aDocument) => {
> +        aDocument.ownerGlobal.BrowserPageActions._placeLazyActionsInPanelNow();

Can we avoid the duplication here by making this call in whatever place calls these `query` methods?

::: browser/modules/PageActions.jsm:38
(Diff revision 13)
>  const PERSISTED_ACTIONS_CURRENT_VERSION = 1;
>  
>  // Escapes the given raw URL string, and returns an equivalent CSS url()
>  // value for it.
>  function escapeCSSURL(url) {
> -  return `url("${url.replace(/[\\\s"]/g, encodeURIComponent)}")`;
> +  return `url("${url.replace(/[\\\s"]/g, encodeURIComponent)}")`; // "

Nit: please remove the added empty comment here.

::: browser/modules/PageActions.jsm:129
(Diff revision 13)
>        }
> -      actions.push(...this.nonBuiltInActions);
> +      actions.push(...nonBuiltInActions);
> +    }
> +    let transientActions =
> +      this._builtInActions.filter(filterTransient)
> +      .concat(this._nonBuiltInActions.filter(filterTransient));

Does it really make sense to have both builtin and non-builtin transient actions? Also, can't we just store the transient actions in a separate list (so have a third array member in addition to `_builtInActions` and `nonBuiltInActions`, avoiding all this runtime processing every time someone asks for the list of actions? This code iterates over each list of actions twice, which seems like it shouldn't be necessary - an action's transient-ness doesn't change, after all...

::: browser/modules/PageActions.jsm:716
(Diff revision 13)
>    setTooltip(value, browserWindow = null) {
>      return this._setProperty("tooltip", value, browserWindow);
>    },
>  
>    /**
> +   * Whether the action wants a subview (bool, nonnull)

What does the (bool, nonnull) mean here?

::: browser/modules/PageActions.jsm:722
(Diff revision 13)
> +   */
> +  getWantsSubview(browserWindow = null) {
> +    return !!this._getProperties(browserWindow).wantsSubview;
> +  },
> +  setWantsSubview(value, browserWindow = null) {
> +    return this._setProperty("wantsSubview", !!value, browserWindow);

If we set this to `false`, does the subview node stay in the document? And then won't it not get removed once the action gets removed, because the action's wantsSubview property is then false?

::: browser/modules/PageActions.jsm:1037
(Diff revision 13)
> +   * @return True if the action should be shown and false otherwise.  Actions
> +   *         are always shown in the panel unless they're both transient and
> +   *         disabled.
> +   */
> +  shouldShowInPanel(browserWindow) {
> +    return !this.__transient || !this.getDisabled(browserWindow);

There are only 3 callsites, and 2 have specific requirements for `this.__transient` themselves. So I think it would be clearer to just inline this check in the callsites.

::: browser/modules/test/browser/browser_PageActions.js:400
(Diff revision 13)
> +  EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
> +  await promisePageActionPanelHidden();

You could just not close the panel here and keep it open until you click the item some 20 lines below?
Attachment #8949631 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8961608 [details]
Bug 1221539 - Add search engine discovery to the page action menu. Part 2: Add the new action.

https://reviewboard.mozilla.org/r/230472/#review237056

r=me with the telemetry thing sorted

::: toolkit/components/telemetry/Histograms.json:6431
(Diff revision 6)
>      "labels": ["bookmark", "pocket", "screenshots", "webcompat", "copyURL", "emailLink",
> -               "sendToDevice", "other"],
> +               "sendToDevice", "addSearchEngine", "other"],

I'm not sure this works - the telemetry docs aren't clear on whether you can add labels mid-array or if new items have to go at the end (which would be unfortunate because of course we want 'other' to be the last item). Please get clarification from the telemetry folks on this before shipping.
Attachment #8961608 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #91)
> Comment on attachment 8961608 [details]
> Bug 1221539 - Add search engine discovery to the page action menu. Part 2:
> Add the new action.
> 
> https://reviewboard.mozilla.org/r/230472/#review237056
> 
> r=me with the telemetry thing sorted
> 
> ::: toolkit/components/telemetry/Histograms.json:6431
> (Diff revision 6)
> >      "labels": ["bookmark", "pocket", "screenshots", "webcompat", "copyURL", "emailLink",
> > -               "sendToDevice", "other"],
> > +               "sendToDevice", "addSearchEngine", "other"],
> 
> I'm not sure this works - the telemetry docs aren't clear on whether you can
> add labels mid-array or if new items have to go at the end (which would be
> unfortunate because of course we want 'other' to be the last item). Please
> get clarification from the telemetry folks on this before shipping.

Yeah, this doesn't work - we can't insert before existing items, only append new labels at the end. r=me to either just append or to append two entries, moving "other" back to the end, see discussion in:

https://mozilla.logbot.info/telemetry/20180327
(In reply to :Gijs from comment #90)
> ::: browser/base/content/browser-pageActions.js:52
> (Diff revision 13)
> >     * Inits.  Call to init.
> >     */
> >    init() {
> >      this.placeAllActions();
> > +    this.panelNode.addEventListener("popupshowing", () => {
> > +      this.mainButtonNode.setAttribute("open", "true");
> 
> Doing this immediately when the item is clicked helps with not having the
> button state flicker from :hover-> :active -> :hover -> [open] . Can we
> continue doing that?

Done, added setAttribute("open") back to showPanel

> ::: browser/base/content/browser-pageActions.js:53
> (Diff revision 13)
> >     */
> >    init() {
> >      this.placeAllActions();
> > +    this.panelNode.addEventListener("popupshowing", () => {
> > +      this.mainButtonNode.setAttribute("open", "true");
> > +      this._onPanelShowing();
> 
> If we move the `open` attribute setting, then in init you can do something
> like:
> 
> ```js
> this._onPanelShowing = this._onPanelShowing.bind(this);
> this.panelNode.addEventListener("popupshowing", this._onPanelShowing);
> ```
> 
> Rather than having some code directly in the event listener and some code in
> the `_onPanelShowing` method that then doesn't have any other callers.

Done

> ::: browser/base/content/browser-pageActions.js:68
> (Diff revision 13)
> > +      let buttonNode = this.panelButtonNodeForActionID(action.id);
> > +      action.onShowingInPanel(buttonNode);
> > +    }
> >    },
> >  
> > +  _placeLazyActionsInPanelNow() {
> 
> As noted in UITour, I think this should probably be public if we call it
> from outside here. I also think that the `Now` bit doesn't really add
> anything - you expect a method called `doThing` to do the thing now, unless
> it explicitly says `doThingAsync` or `doThingLazy` or whatever.

Done

> ::: browser/base/content/browser-pageActions.js:89
> (Diff revision 13)
> > -    // Place actions in the urlbar.  Do this in reverse order.  The reason is
> > +    for (let action of urlbarActions) {
> > -    // subtle.  If there were no urlbar nodes already in markup (like the
> > -    // bookmark star button), then doing this in forward order would be fine.
> > -    // Forward order means that the insert-before relationship is always broken:
> > -    // there's never a next-sibling node before which to insert a new node, so
> > -    // node.insertBefore() is always passed null, and nodes are always appended.
> > -    // That will break the position of nodes that should be inserted before
> > -    // nodes that are in markup, which in turn can break other nodes.
> > -    let actionsInUrlbar = PageActions.actionsInUrlbar(window);
> 
> Why do we no longer need to do this in reverse order as this comment
> indicates? I still see the star-button-box in browser.xul in DXR...

Previously, insertBeforeNode() was a method on PageActions, and all it did was return the ID of the action after a given action in either the panel or urlbar array.  It didn't check to see whether that next action was actually present in the DOM.  _getNextNode on the other hand does check whether the next action is in the DOM.  In fact it returns the ID of the next closest action that's in the DOM.  So there's no longer any danger of nodes ending up in the wrong order.

> ::: browser/base/content/browser-pageActions.js:112
> (Diff revision 13)
> >     *
> >     * @param  action (PageActions.Action, required)
> >     *         The action to place.
> >     */
> >    placeActionInPanel(action) {
> > +    if (this.panelNode.state == "open") {
> 
> Maybe it'd be safer to have this be `!= "closed"` so that we also do this if
> someone adds an action from a popupshowing handler or similar (when state ==
> "showing") ?

Done

> ::: browser/base/content/browser-pageActions.js:205
> (Diff revision 13)
> > +   *         return the panel button ID.  If you're inserting into the urlbar,
> > +   *         then it should return the urlbar button ID.
> > +   * @return (DOM node, maybe null) The DOM node before which to insert the
> > +   *         given action.  Null if the action should be inserted at the end.
> > +   */
> > +  _insertBeforeNode(action, actionArray, nodeGetterFn) {
> 
> This name implies we're inserting the node - can we name it `getNextNode` or
> something?

Done, _getNextNode

> Also, the node getter function seems overkill - can we just pass a bool or
> string that indicates whether this is a panel/urlbar node we're looking for?
> Then push the actual fetching of the id into this method. This would also
> mean we can push the actionArray getting into this method, because both
> callsites just fetch that list and then don't do anything with it.

Done

> ::: browser/base/content/browser-pageActions.js:659
> (Diff revision 13)
> > +      if (panelViewNode) {
> > +        panelViewNode.remove();
> > +      }
> > +      return;
> > -      }
> > +    }
> > -      node.setAttribute("tooltiptext", tooltip || title);
> > +    panelNode.classList.add("subviewbutton-nav");
> 
> Move this above the `if (!wantsSubview)` block and just use:
> 
> ```js
> panelNode.classList.toggle("subviewbutton-nav", wantsSubview);
> ```
> 
> (and remove the identical call inside the if block)

Done

> ::: browser/base/content/browser-pageActions.js:688
> (Diff revision 13)
> > -    if (action.subview || action.wantsIframe) {
> > +    if (!aaPanelNode || aaPanelNode.getAttribute("actionID") != action.id) {
> > +      action.onCommand(event, buttonNode);
> > +    }
> 
> I'm very confused - how does `onSubviewShowing` still get called if the
> action's `onCommand` changes its `wantsSubview` property? It looks like
> that's supposed to happen above this code, and the `togglePanelForAction`
> code will cause the panel to close and then reopen showing only the relevant
> subview. I don't think that's what we want.

The part above this code handles the subview-showing case for actions in the panel.  This code here handles the case for actions in the urlbar.  togglePanelForAction ensures that onSubviewShowing is called for such actions.

> ::: browser/components/uitour/UITour.jsm:161
> (Diff revision 13)
> >          // It would be hidden if toggled off from the urlbar.
> >          let node = aDocument.getElementById("pocket-button-box");
> >          if (node && !node.hidden) {
> >            return node;
> >          }
> > +        aDocument.ownerGlobal.BrowserPageActions._placeLazyActionsInPanelNow();
> 
> This feels a bit like a code smell... At a minimum, it seems like this
> should be a public method, given it's called from other modules.

Made public (the only thing this is doing is forcing the panel to update itself non-lazily, which is necessary in this one case because UITour gets the nodes it needs to highlight before it actually opens the panel)

> ::: browser/components/uitour/UITour.jsm:255
> (Diff revision 13)
> >                 aDocument.getElementById("pageAction-panel-sendToDevice");
> >        },
> >      }],
> >      ["screenshots", {
> >        query: (aDocument) => {
> > +        aDocument.ownerGlobal.BrowserPageActions._placeLazyActionsInPanelNow();
> 
> Can we avoid the duplication here by making this call in whatever place
> calls these `query` methods?

Not really because query() is called for all kinds of things, not only the page action panel, so it's really the right thing to do to call this in each of these methods

> ::: browser/modules/PageActions.jsm:38
> (Diff revision 13)
> >  const PERSISTED_ACTIONS_CURRENT_VERSION = 1;
> >  
> >  // Escapes the given raw URL string, and returns an equivalent CSS url()
> >  // value for it.
> >  function escapeCSSURL(url) {
> > -  return `url("${url.replace(/[\\\s"]/g, encodeURIComponent)}")`;
> > +  return `url("${url.replace(/[\\\s"]/g, encodeURIComponent)}")`; // "
> 
> Nit: please remove the added empty comment here.

Done

> ::: browser/modules/PageActions.jsm:129
> (Diff revision 13)
> >        }
> > -      actions.push(...this.nonBuiltInActions);
> > +      actions.push(...nonBuiltInActions);
> > +    }
> > +    let transientActions =
> > +      this._builtInActions.filter(filterTransient)
> > +      .concat(this._nonBuiltInActions.filter(filterTransient));
> 
> Does it really make sense to have both builtin and non-builtin transient
> actions? Also, can't we just store the transient actions in a separate list
> (so have a third array member in addition to `_builtInActions` and
> `nonBuiltInActions`, avoiding all this runtime processing every time someone
> asks for the list of actions? This code iterates over each list of actions
> twice, which seems like it shouldn't be necessary - an action's
> transient-ness doesn't change, after all...

Done, now we have three arrays

> ::: browser/modules/PageActions.jsm:716
> (Diff revision 13)
> >    setTooltip(value, browserWindow = null) {
> >      return this._setProperty("tooltip", value, browserWindow);
> >    },
> >  
> >    /**
> > +   * Whether the action wants a subview (bool, nonnull)
> 
> What does the (bool, nonnull) mean here?

This property is a bool that will never be nonnull.  Some properties for example can be null even though their primary type is, like, a string or something.  Anyway I removed all these nonnull/nullable things

> ::: browser/modules/PageActions.jsm:722
> (Diff revision 13)
> > +   */
> > +  getWantsSubview(browserWindow = null) {
> > +    return !!this._getProperties(browserWindow).wantsSubview;
> > +  },
> > +  setWantsSubview(value, browserWindow = null) {
> > +    return this._setProperty("wantsSubview", !!value, browserWindow);
> 
> If we set this to `false`, does the subview node stay in the document? And
> then won't it not get removed once the action gets removed, because the
> action's wantsSubview property is then false?

No, the node is removed in BrowserPageActions via the updateAction and _updateActionWantsSubview path

> ::: browser/modules/PageActions.jsm:1037
> (Diff revision 13)
> > +   * @return True if the action should be shown and false otherwise.  Actions
> > +   *         are always shown in the panel unless they're both transient and
> > +   *         disabled.
> > +   */
> > +  shouldShowInPanel(browserWindow) {
> > +    return !this.__transient || !this.getDisabled(browserWindow);
> 
> There are only 3 callsites, and 2 have specific requirements for
> `this.__transient` themselves. So I think it would be clearer to just inline
> this check in the callsites.

I left this as is because the shouldShowInPanel determination depends on both action._transient and action.getDisabled() (and because the two callsites you mention are rewritten/gone), and there's an analogous shouldShowInUrlbar, and one caller is in BrowserPageActions, so I think it makes more sense to have this

> ::: browser/modules/test/browser/browser_PageActions.js:400
> (Diff revision 13)
> > +  EventUtils.synthesizeMouseAtCenter(BrowserPageActions.mainButtonNode, {});
> > +  await promisePageActionPanelHidden();
> 
> You could just not close the panel here and keep it open until you click the
> item some 20 lines below?

Done
(In reply to :Gijs from comment #92)
> Yeah, this doesn't work - we can't insert before existing items, only append
> new labels at the end. r=me to either just append or to append two entries,
> moving "other" back to the end, see discussion in:
> 
> https://mozilla.logbot.info/telemetry/20180327

I moved the new label to the end, thanks for catching this
(In reply to Drew Willcoxon :adw from comment #97)
> > ::: browser/components/uitour/UITour.jsm:255
> > (Diff revision 13)
> > >                 aDocument.getElementById("pageAction-panel-sendToDevice");
> > >        },
> > >      }],
> > >      ["screenshots", {
> > >        query: (aDocument) => {
> > > +        aDocument.ownerGlobal.BrowserPageActions._placeLazyActionsInPanelNow();
> > 
> > Can we avoid the duplication here by making this call in whatever place
> > calls these `query` methods?
> 
> Not really because query() is called for all kinds of things, not only the
> page action panel,

Sure, but we could just make the callsite of query() call this when the target is any of screenshots/pocket/pageAction* . Anyway, I guess this is fine... it's just a bit ugly, but that goes for a lot of UITour...
Comment on attachment 8949631 [details]
Bug 1221539 - Add search engine discovery to the page action menu. Part 1: Page action changes.

https://reviewboard.mozilla.org/r/218982/#review237446
Attachment #8949631 - Flags: review?(gijskruitbosch+bugs) → review+
Rebased on current tree, fixed a few failures in browser_PageActions.js due to the addition of PageActions._transientActions in the previous patch.  One more push to try before landing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b921db4da3abcafe0b9955eeb703e987fbefc79e
Forgot to update a method name in UITour.jsm.  I'll land this now.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88abb5bd6790
Add search engine discovery to the page action menu. Part 1: Page action changes. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c6c9e8f68dd8
Add search engine discovery to the page action menu. Part 2: Add the new action. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/88abb5bd6790
https://hg.mozilla.org/mozilla-central/rev/c6c9e8f68dd8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Don't know if this is already fixed in the bug fix mentioned in comment 103, but in the current nightly, if i go to the 
page actions > right click on the search engine > and choose "Add to Address Bar" 
i get a blank space/button on the address bar, instead of the icon to add a new search engine.
If i click on it, nothing happens
(In reply to Maverick from comment #109)
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101
> Firefox/61.0
> 
> Don't know if this is already fixed in the bug fix mentioned in comment 103,
> but in the current nightly, if i go to the 
> page actions > right click on the search engine > and choose "Add to Address
> Bar" 
> i get a blank space/button on the address bar, instead of the icon to add a
> new search engine.
> If i click on it, nothing happens

Please file a separate bug, and indicate what site you're seeing this behaviour on.
Flags: needinfo?(johnmaverick74)
(In reply to :Gijs from comment #110)
> (In reply to Maverick from comment #109)
> > Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101
> > Firefox/61.0
> > 
> > Don't know if this is already fixed in the bug fix mentioned in comment 103,
> > but in the current nightly, if i go to the 
> > page actions > right click on the search engine > and choose "Add to Address
> > Bar" 
> > i get a blank space/button on the address bar, instead of the icon to add a
> > new search engine.
> > If i click on it, nothing happens
> 
> Please file a separate bug, and indicate what site you're seeing this
> behaviour on.

Added as bug 1449947
Screenshot attached
Flags: needinfo?(johnmaverick74)
Depends on: 1449947
Depends on: 1450125
Drew, since this is a visible change in the UI for users, could you please nominate this bug for release note addition? Thanks
Flags: needinfo?(adw)
Depends on: 1450294
Please add dot on "Page action" button, when in this menu is someting not a permanent menuitem.

This may refer not only to "add search engine" but each  not a permanent menuitem added from eextenisons to "Page action menu".

What do you think?
(In reply to Łukasz from comment #113)
> Please add dot on "Page action" button, when in this menu is someting not a
> permanent menuitem.
> 
> This may refer not only to "add search engine" but each  not a permanent
> menuitem added from eextenisons to "Page action menu".
> 
> What do you think?

Imho, while i understand the idea, in this particular case (adding a new search engine), when you have one available you can just right-click it and choose "Add to address bar". 
This will imply that everytime a new search engine is available to be added an icon for it will show up on the address bar.

(this is also available for other not-permanent item features)

Still, if believed that a badge from the 3 dot's is needed i would recommend adding other badge that is not a dot (because then you'll have 4), or paint one of the 3 in a different color... but i believe it's important to keep the 3 dots "image"
Release Note Request (optional, but appreciated)
[Why is this notable]:
Before this bug, the only way to install OpenSearch plugins offered by web pages was via the search bar.  If you hid your search bar (and all new profiles have the search bar hidden by default), then there was no way for you to install new search engines offered by web pages.  With this bug, OpenSearch plugins are now offered in the page action menu (in addition to the search bar, as before).

[Affects Firefox for Android]:
No

[Suggested wording]:
OpenSearch plugins offered by web pages can now be added from the page action menu.

[Links (documentation, blog post, etc)]:
None
relnote-firefox: --- → ?
Flags: needinfo?(adw)
(In reply to Drew Willcoxon :adw from comment #115)
> [Suggested wording]:
> OpenSearch plugins offered by web pages can now be added from the page
> action menu.

Thank you, this was added to Nightly release notes. For beta and final release notes, our release managers will decide on the integration and wording.
(In reply to Maverick from comment #114)
> Still, if believed that a badge from the 3 dot's is needed i would recommend
> adding other badge that is not a dot (because then you'll have 4), or paint
> one of the 3 in a different color... but i believe it's important to keep
> the 3 dots "image"

The last dot could be slightly enlarged. Or have an extra border. Anything.
Depends on: 1451119
(In reply to Łukasz from comment #117)
> The last dot could be slightly enlarged. Or have an extra border. Anything.

I filed bug 1451119 for a badge or other kind of indicator on the primary page action button.
I opened Bug 1452700 as a followup.  The green pop up isn't providing enough context to the location of the new search engine.
Blocks: 1452700
While we are working on the opensearch integration... Someone should fix the automated updating of opensearch documents. For some time I observed that documents (opensearch) that are long outdated dont get automatically updated by firefox.

It is extremely annoying for users if you've added a search but it does not work. Since there is currently no interface (known to me) that allows a websites (owners) to force an update of opensearch documents, it must/should be handeled internally by firefox, while it is part of the opensearch standard.

AND a better documentation on the update behavior must be added.

..... Please correct me if I missed something
Flags: needinfo?(adw)
Please file a new bug
Flags: needinfo?(adw)
> Someone should fix the automated updating of opensearch documents.

We already have support for this if the engine specifies an UpdateURL.

https://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1980
(In reply to Mike Kaply [:mkaply] from comment #122)
> > Someone should fix the automated updating of opensearch documents.
> 
> We already have support for this if the engine specifies an UpdateURL.
> 
> https://searchfox.org/mozilla-central/source/toolkit/components/search/
> nsSearchService.js#1980

Thank you. I have not worked on firefox so far. Had not found this code yet. Unfortunately could observe any updates so far.
Hi Drew, Is this a qe-verify bug or needs a test plan? Can you raise the appropriate flag for action?

Thanks!
Flags: needinfo?(adw)
Flags: needinfo?(adw) → qe-verify+
In the "Page actions" dropdown, for any search engine, the text shown is: Add Search Engine (without the name of the engine or any more information - not as in the specs). The icon of the engine is correctly shown, but I also tried Amazon for different countries and only one engine appears at a time (can't make it to appear more engines in the drop down). For the same case with Amazon, if I add country x to the list and then I visit Amazon from another country y, I cannot add y to the list (so only from one country can be added).

Marco, my question is: regarding what I wrote above, is the objective of the issue completed and the things that are not working as expected will be fixed in other issues?
Flags: needinfo?(mak77)
Also:

- there are search engines that don't appear in the "Page actions" dropdown. Ex: dogpile, google
- when a new engine appears in the "Page actions", and it is being added to the list by clicking on it, it remains in the "Page actions" and can be selected again (even if in this case a popup appears with the message: Nightly could not install the search plugin from “https://ro.search.yahoo.com/opensearch.xml” because an engine with the same name already exists.)
(In reply to David Olah from comment #126)
> Also:
> 
> - there are search engines that don't appear in the "Page actions" dropdown.
> Ex: dogpile, google

Only search engines that are semantically linked from the page can show up (neither dogpile nor google do this).

> - when a new engine appears in the "Page actions", and it is being added to
> the list by clicking on it, it remains in the "Page actions" and can be
> selected again (even if in this case a popup appears with the message:
> Nightly could not install the search plugin from
> “https://ro.search.yahoo.com/opensearch.xml” because an engine with the same
> name already exists.)

This seems like a bug. Please file a new bug as a dependency, provide full steps to reproduce from a clean profile, and ni me or :adw there.
(In reply to :Gijs (he/him) from comment #127)
> This seems like a bug. Please file a new bug as a dependency, provide full
> steps to reproduce from a clean profile, and ni me or :adw there.

Yahoo is a special case.  The name of the engine in the opensearch plugin doesn't match the final name that the engine gets installed with, so Firefox can't currently tell that they're the same engine.  It happens with the search bar, too.  IIRC there's a bug on file for fixing that problem in general.
Flags: needinfo?(mak77)
(In reply to David Olah from comment #125)
> In the "Page actions" dropdown, for any search engine, the text shown is:
> Add Search Engine (without the name of the engine or any more information -
> not as in the specs).

This was changed in bug 1450294 so it's expected.

> The icon of the engine is correctly shown, but I also
> tried Amazon for different countries and only one engine appears at a time
> (can't make it to appear more engines in the drop down). For the same case
> with Amazon, if I add country x to the list and then I visit Amazon from
> another country y, I cannot add y to the list (so only from one country can
> be added).

We identify search engines by title, so if all the amazon search engines have the same title then I don't think this is surprising. Looking at the source code of amazon.co.uk, amazon.com and amazon.de, all of the pages point to the same engine file ( https://d2lo25i6d3q8zm.cloudfront.net/browser-plugins/AmazonSearchSuggestionsOSD.Firefox.xml - search for 'opensearch' in view-source to check) and so AFAICT all of these pages advertise the exact same search engine. Are you seeing something else? If so, please provide more detailed steps in a separate follow-up bug.
Flags: needinfo?(david.olah)
(In reply to Drew Willcoxon :adw from comment #128)
> (In reply to :Gijs (he/him) from comment #127)
> > This seems like a bug. Please file a new bug as a dependency, provide full
> > steps to reproduce from a clean profile, and ni me or :adw there.
> 
> Yahoo is a special case.  The name of the engine in the opensearch plugin
> doesn't match the final name that the engine gets installed with, so Firefox
> can't currently tell that they're the same engine.  It happens with the
> search bar, too.  IIRC there's a bug on file for fixing that problem in
> general.

Huh. There's bug 1222107 and bug 1352445 but nothing that really match this that I can find... would be good to hook that up to this bug as a 'see also' issue, though I fully agree that it doesn't really change anything given the search bar has the same issue.
Yeah, Florian's https://bugzilla.mozilla.org/show_bug.cgi?id=1222107#c5 addresses it, and it seems to be what bug 351441 is about, but it was wontfixed. :-/  We should revisit that...  I'm not sure if there's another bug or not.

(And my description of the problem was wrong.  It's when the title in the HTML <link> doesn't match the title in the opensearch xml.)
(In reply to Drew Willcoxon :adw from comment #131)
> Yeah, Florian's https://bugzilla.mozilla.org/show_bug.cgi?id=1222107#c5
> addresses it, and it seems to be what bug 351441 is about, but it was
> wontfixed. :-/  We should revisit that...  I'm not sure if there's another
> bug or not.
> 
> (And my description of the problem was wrong.  It's when the title in the
> HTML <link> doesn't match the title in the opensearch xml.)

So, finally I should create a new bug with the issue from comment 127, wright?
Flags: needinfo?(david.olah)
(In reply to David Olah from comment #132)
> (In reply to Drew Willcoxon :adw from comment #131)
> > Yeah, Florian's https://bugzilla.mozilla.org/show_bug.cgi?id=1222107#c5
> > addresses it, and it seems to be what bug 351441 is about, but it was
> > wontfixed. :-/  We should revisit that...  I'm not sure if there's another
> > bug or not.
> > 
> > (And my description of the problem was wrong.  It's when the title in the
> > HTML <link> doesn't match the title in the opensearch xml.)
> 
> So, finally I should create a new bug with the issue from comment 127,
> wright?

I'm not sure - Drew, can you confirm?
Flags: needinfo?(adw)
Yes, could you please file a bug, David?  The only way to fix it afaict would be to prefetch opensearch xml.  (Which would also let us fix a similar bug were the favicons don't match.)
Flags: needinfo?(adw)
Depends on: 1457069
Sure I can. here is Bug 1457069 that I created for this.
As Bug 1457069 from Comment 135 was created, I am marking this issue as verified.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1483167
Depends on: 1489834
No longer depends on: 1489834
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: