1262916 - Option to add custom search engine should disappear when site is added

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Sevaan Franks [:sevaan]]

Issue:

The option to add a custom search engine remains on screen even if a user has added the site.

STR:

- Go to http://us.imdbcom
- Long-press on the search field to bring up the "Add Custom Search" bar at the top of the screen
- Click the Add icon
- Notification appears saying site has been added

What should happen:

After adding the site, the bar at the top of the screen should disappear revealing the URL bar again.

What currently happens:

The bar remains on screen, blocking the URL bar until the user dismisses it.

Comment 1

[:Margaret Leibovic]

This may be different now that Sebastian just landed the floating action bar (bug 1171110).

Comment 2

[Mark Capella [:capella]]

I'd noticed that behaviour during testing of his bug, and aiui, it remains. You wind up with like "Yahoo", "Yahoo 2", which of course you can simply remove, but fixing might be better.

Comment 3

[Mark Capella [:capella]]

Attachment: Option to add custom search engine should disappear when site is added,

Comment 4

[Mark Capella [:capella]]

Comment on attachment 8739484 [details] [diff] [review]
Option to add custom search engine should disappear when site is added,

Dang, forgot to try out Reviewboard this time :/

This seems to work for both new/Floating [0] and old/ActionBar [1]

[0] https://www.dropbox.com/s/nnfsoxl1fyl17oo/bug1262916_Floating.mp4?dl=0
[1] https://www.dropbox.com/s/7li7rtz01u328tq/bug1262916_nonFloating.mp4?dl=0

Comment 5

[Mark Capella [:capella]]

Attachment: MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

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

Comment 6

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #2)
> I'd noticed that behaviour during testing of his bug, and aiui, it remains.
> You wind up with like "Yahoo", "Yahoo 2", which of course you can simply
> remove, but fixing might be better.

We have logic in place to prevent this for adding open search engines through the menu, we could file a follow-up to include this logic for the action bar action as well:
https://dxr.mozilla.org/mozilla-central/rev/d9b1a9829c8ee2862955043f28183efa07de3d2b/mobile/android/chrome/content/browser.js#3898

Comment 7

[:Margaret Leibovic]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

https://reviewboard.mozilla.org/r/45293/#review42385

I think there was some confusion with this bug and patch... my reading of the bug was that we should dismiss the action bar after taking the "add search engine" action.

While I agree it's a nice improvement to not show the "add search engine" action if that engine already exists, that seems like a separate issue to the one this bug was filed about.

::: mobile/android/chrome/content/ActionBarHandler.js:76
(Diff revision 1)
>          if (e.reason == 'visibilitychange' || e.reason == 'presscaret') {
> +          // Visibility changes don't affect boundingClientRect.
>            this._updateVisibility();
>          } else {
> +          // Selection changes update boundingClientRect.
> +          this._boundingClientRect = e.boundingClientRect;

Why do you need to create a `_boundingClientRect` member variable for this patch?

::: mobile/android/chrome/content/ActionBarHandler.js:561
(Diff revision 1)
>          UITelemetry.addEvent("action.1", "actionbar", null, "add_search_engine");
> -        SearchEngines.addEngine(element);
> +
> +        // Engines are added asynch. Keep local "added" state of current element to
> +        // avoid race during  _getActionBarActions() update of selectors,
> +        // then update the Toolbar UI.
> +        ActionBarHandler._searchEngineAdded = SearchEngines.addEngine(element);

This boolean flag seems pretty fragile... what happens if the user quickly dismisses the action bar and then taps on the input box again? The cached flag would be wiped.

::: netwerk/base/nsIBrowserSearchService.idl:284
(Diff revision 1)
> +   * @param url
> +   *        The URL to which search queries should be sent.
> +   *        Must not be null.
> +   *
> +   */
> +  boolean searchEngineForURLExists(in AString method, in AString url);

Do we really need to edit the search service API for this? If we can avoid this, I think we should.

If we can't avoid it, you should also get a review from Florian.

Comment 8

[Mark Capella [:capella]]

re: I think there was some confusion with this bug and patch

   I did make that the initial patch (just close the actionbar / floatingToolbar) as described, but actual use seemed inelegant. Testing it, it seemed the more likely case is that people will tap into an <input> to take some standard action (Cut/Copy/Paste), spot that |Add As Search Engine| option, then quickly decide to do that first, before continuing on to what they had in mind originally.

   I could see the next patch ... "Why do I have to tap twice to do this?" :-) However, that's a trivial change, if you'd prefer it vs. this one.

re: Why do you need to create a `_boundingClientRect`?

   I made the var global, partly for convenience vs. passing it around, but mainly because it's not otherwise available in the ActionBarHandler life-cycle at the point a user taps the |Add As Search Engine| option. ActionBarHandler needs it to broadcast the "TextSelection:ActionbarStatus" message back to FloatingToolbarTextSelection to update the UI.

   I could have had ActionBarHandler send empty info in this case, and FloatingToolbarTextSelection default to re-using it's own version provided by the previous message, as the boundaries of the selected text clientRect can't change in between without causing it to receive an automatic update otherwise.

re: This boolean flag seems pretty fragile...

   It does. I've assumed that once the user dismisses the selection, then navigates to a new visible ActionBar/FloatingTool state via 1) long-press into populated or empty editable, or 2) Tap into editable followed by tap of orange AccessibleCaret, that the async update will have completed, and the call to |SearchEngines.visibleEngineExists(element)| in SEARCH_ADD.selector will be valid. I can provide average completion times of that async task if you'd like.

re: Do we really need to edit the search service API?

   I think so, and expected to follow with review from someone appropriate. Thanks for identifying Florian! And as you mention earlier, we'll also look to re-use it in browser for the change to sendOpenSearchMessage().

   While we can get an nsISearchEngine from the service, the EngineURL() objects are stored internally to it in a |this._urls| array that don't seem exposed externally. |_getURLOfType()| and |supportsResponseType()| come close, but both fail our needs.

   We're adding a method (|searchEngineForURLExists()| which I plan to rename |hasEngineURL()|), to scan those read-only, then of course get review/permission for the concept from Florian. I'll update as noted here, and ping him for review before returning to you for the final.

Comment 9

[Mark Capella [:capella]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

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

Comment 10

[Mark Capella [:capella]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

obsolete

Comment 11

[Mark Capella [:capella]]

So florian doesn't come in blind, I'm asking review for the search service API change as per comment 8 above.

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

https://reviewboard.mozilla.org/r/45293/#review44153

It took me a while to understand what's going on. I was initially going to ask why you can't just do something similar to what Firefox desktop does, but I had never seen that Fennec creates search plugins from forms (since bug 725213). Looks like half of what we want to do on Firefox desktop in bug 648398 is already done for Fennec! :-)

Adding an exposed method to make this work correctly seems OK to me. I'm not convinced yet that code currently in the patch actually does what you want, especially when using the POST method.

::: netwerk/base/nsIBrowserSearchService.idl:277
(Diff revision 2)
> +   * Checks if an EngineURL of type URLTYPE_SEARCH_HTML exists for
> +   * any engine, with a matching method and template URL.
> +   *
> +   * @param method
> +   *        The HTTP request method used when submitting a search query.
> +   *        Must be a case insensitive value of either "get" or "post".

The rest of the code in the search service seems to be using uppercase methods, eg. "GET".

Is your implementation somehow missing .toLowerCase() calls to make the comparison case insensitive?

::: toolkit/components/search/nsSearchService.js:4366
(Diff revision 2)
> +  hasEngineURLforMethod: function SRCH_SVC_existsSHEU(aMethod, aTemplate) {
> +    this._ensureInitialized();
> +
> +    return this._getSortedEngines(false).some((engine) => {
> +      let urlsLength = engine._urls.length;
> +      for (let i = 0; i < urlsLength; i++) {

for (let url of engine._urls)

::: toolkit/components/search/nsSearchService.js:4370
(Diff revision 2)
> +      let urlsLength = engine._urls.length;
> +      for (let i = 0; i < urlsLength; i++) {
> +        let engineURL = engine._urls[i];
> +        if (engineURL.type == URLTYPE_SEARCH_HTML &&
> +            engineURL.method == aMethod &&
> +            engineURL.template == aTemplate) {

Checking only the template is probably not enough if you have a website with several different forms POSTing to the same base URL but with different parameters.

Comment 13

[Mark Capella [:capella]]

Ok, this new patch is much cleaner. Thanks to you both for the comments on the first review pass.


@margaret: update from comment #8 ... I've replaced the temp var with an asynch callback to update the Selection UI.

Also, you'll probably notice a change in ActionBarHandler.SEARCH_ADD.selector() dropping a check for |method == ""|. That bit isn't actually correct, as nsSearchService considers that invalid [0]. It looks like a bit of cargo-cruft from SelectionHandler.js [1], and InlineSpellChecker.jsm [2] before that.

[0] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=419922c1f15f&mark=1085-1085,1092-1093#1064
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?mark=821-822#794
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm?rev=f1d82662fe93&mark=434-434,454-455#433


@florian: update from comment #12 ... I address the nits, and have improved the the |hasEngineURL()| to take all EngineURL queryParms into consideration.

All of the search forms I see and tested were for GET methods, so I may need more information re: POST methods.

I looked over and cc:ed onto bug 648398 ... let me know if I can do something to help there.

Comment 14

[Mark Capella [:capella]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

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

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

https://reviewboard.mozilla.org/r/45293/#review45807

::: netwerk/base/nsIBrowserSearchService.idl:286
(Diff revision 3)
> +   *        Must not be null.
> +   *
> +   * @param formData
> +   *        The un-sorted form data used as query params.
> +   */
> +  boolean hasEngineURL(in AString method, in AString url, in jsval formData);

This new method needs to be covered by xpcshell tests.

::: toolkit/components/search/nsSearchService.js:4361
(Diff revision 3)
>  
> +  /**
> +   * Checks to see if any engine has an EngineURL of type URLTYPE_SEARCH_HTML
> +   * for this request-method, template URL, and query params.
> +   */
> +  hasEngineURL: function(method, template, formData) {

I see 2 edge cases that aren't handled:
- if there are several parameters with the same name but different values (the sort done here doesn't guarantee these will always be in the same order for the comparison)
- when using the GET method, if some parameters are already provided in the template URL, we may have a duplicate we won't catch.

I'm not sure these cases are worth handling, but I figured I would mention them in the review anyway.

::: toolkit/components/search/nsSearchService.js:4365
(Diff revision 3)
> +   */
> +  hasEngineURL: function(method, template, formData) {
> +    this._ensureInitialized();
> +
> +    // Ensure provided formData is pre-sorted.
> +    let sortedFormData = formData.filter((a) => { return (a.name && a.value); }).

.filter((a) => { return (a.name && a.value); })
can be simplified to:
.filter(a => a.name && a.value)

::: toolkit/components/search/nsSearchService.js:4366
(Diff revision 3)
> +  hasEngineURL: function(method, template, formData) {
> +    this._ensureInitialized();
> +
> +    // Ensure provided formData is pre-sorted.
> +    let sortedFormData = formData.filter((a) => { return (a.name && a.value); }).
> +      sort((a, b) => {

nit: Start the line with the '.', and align it with the '.' before 'filter'.

::: toolkit/components/search/nsSearchService.js:4367
(Diff revision 3)
> +    this._ensureInitialized();
> +
> +    // Ensure provided formData is pre-sorted.
> +    let sortedFormData = formData.filter((a) => { return (a.name && a.value); }).
> +      sort((a, b) => {
> +       return (a.name > b.name) ? 1 : (b.name > a.name) ? -1 : 0;

nit: one missing space of indent here.

::: toolkit/components/search/nsSearchService.js:4384
(Diff revision 3)
> +            url.params.length != sortedFormLength) {
> +          return false;
> +        };
> +
> +        // Ensure engineURL formData is pre-sorted.
> +        let sortedParams = url.params.filter((a) => { return (a.name && a.value); }).

This filter/sort code is duplicated with the code a few lines above. How about creating a helper function used in both cases?

Comment 16

[Mark Capella [:capella]]

@florian: Nice!

Updating the sorts for the dup name/value possibility was a good idea, and not difficult. In the case of queryParams embedded in the URL, I think this is correct.

I tried it, and after adding a search engine from the first bookmark with no URL-queryParms, the method does flag as a dup on navigation via a different bookmark WITH URL-queryParams. Basically, to the user, he isn't shown a Selection UI option to "Add To Search Engine" the second time, as I'd expected.

But if you decide later, there is something subtle here that's worth handling, I'm happy to followup. In any case we're safe in mobile. If we allow users to add dup entries, there is a Settings UI where they can delete them, and I don't see us now blocking addition of search engines erroneously.

I've deviated a little from your code styling suggestion, as after tweaking the sort() and the helperMethod, it looked a little odd. The attached version reads easily, and would be fine in the mobile codebase. Let me know if there's a show stopper / required changes yet for:
toolkit/components/search

Finally, I added a basic xpcshell test. Luckily they're pretty close to my more familiar robocop tests these days, and it rolled out pretty quick.

This is starting to feel pretty good. ( I hope you agree :-) ) Let me know?

Comment 17

[Mark Capella [:capella]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45293/diff/3-4/

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

https://reviewboard.mozilla.org/r/45293/#review46045

Looks reasonable for the part I reviewed. Please push to try before landing.

::: netwerk/base/nsIBrowserSearchService.idl:286
(Diff revision 4)
> +   *        Must not be null.
> +   *
> +   * @param formData
> +   *        The un-sorted form data used as query params.
> +   */
> +  boolean hasEngineURL(in AString method, in AString url, in jsval formData);

Would the name "hasEngineWithURL" be slightly more explicit?

::: toolkit/components/search/nsSearchService.js:4365
(Diff revision 4)
> +   */
> +  hasEngineURL: function(method, template, formData) {
> +    this._ensureInitialized();
> +
> +    // Quick helper method to ensure formData filtered/sorted for compares.
> +    let getSortedFormData = (data) => {

nit: "(data)" can become just "data" here.

::: toolkit/components/search/tests/xpcshell/test_hasEngineURL.js:26
(Diff revision 4)
> +}
> +
> +// Helper method, check engine does or doesn't exist.
> +function checkEngineState(exists, engine) {
> +  do_check_true(exists ===
> +    Services.search.hasEngineURL(engine.method, engine.formURL, engine.queryParams));

Looks like this wants to be:
  do_check_eq(exists, Services.search.hasEngineURL...

::: toolkit/components/search/tests/xpcshell/test_hasEngineURL.js:32
(Diff revision 4)
> +}
> +
> +
> +// Main test.
> +add_task(function* test_hasEngineURL() {
> +

You likely want to start the test with:
  yield asyncInit();
If you don't, I think you'll see a warning about some code causing a deprecated synchronous initialization of the search service.

::: toolkit/components/search/tests/xpcshell/test_hasEngineURL.js:104
(Diff revision 4)
> +
> +
> +  // Add the unsorted engine and it's queryParams.
> +  Services.search.addEngineWithDetails(UNSORTED_ENGINE.name, null, null, null,
> +                                       UNSORTED_ENGINE.method, UNSORTED_ENGINE.formURL);
> +  yield promiseAfterCache();

Why is this needed?

Comment 19

[Mark Capella [:capella]]

Updated, and push to TRY (Linux & Android, XPCShell & robocop tests) looks great
https://treeherder.mozilla.org/#/jobs?repo=try&revision=632ba138f2e8266f3b28e8e40b8741806074ac63

Comment 20

[:Margaret Leibovic]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

https://reviewboard.mozilla.org/r/45293/#review47737

::: mobile/android/chrome/content/browser.js:6652
(Diff revision 4)
> +   */
> +  _getSortedFormData: function(element) {
>      let formData = [];
>  
> -    for (let i = 0; i < form.elements.length; ++i) {
> -      let el = form.elements[i];
> +    for (let formElement of element.form.elements) {
> +      if (!formElement.type) {

These variable renames don't seem necessary as part of this patch. In general, please put cleanup changes in a separate commit.

::: mobile/android/chrome/content/browser.js:6689
(Diff revision 4)
> +    };
> +
> +    // nsIBrowserSearchService.hasEngineURL() ensures sort, but this helps.
> +    return formData.filter(a => a.name && a.value).sort((a, b) => {
> +      return (a.name > b.name) ? 1 : (b.name > a.name) ? -1 :
> +             (a.value > b.value) ? 1 : (b.value > a.value) ? -1 : 0;

This is really hard for me to follow... could you update this to use if statements instead of chaining together all of these ternary operators?

::: mobile/android/chrome/content/browser.js:6710
(Diff revision 4)
> +    let formURL = Services.io.newURI(form.getAttribute("action"), charset, docURI).spec;
> +
> +    return Services.search.hasEngineURL(method, formURL, formData);
> +  },
> +
> +  addEngine: function addEngine(aElement, resultCallback) {

Please add documentation about what type of function you expect this resultCallback to be. And what the return value for this function means.

::: mobile/android/chrome/content/browser.js:6722
(Diff revision 4)
>  
>      // prompt user for name of search engine
>      let promptTitle = Strings.browser.GetStringFromName("contextmenu.addSearchEngine3");
>      let title = { value: (aElement.ownerDocument.title || docURI.host) };
> -    if (!Services.prompt.prompt(null, promptTitle, null, title, null, {}))
> -      return;
> +    if (!Services.prompt.prompt(null, promptTitle, null, title, null, {})) {
> +      return (resultCallback) ? resultCallback(false) : false;

Why are you updating `addEngine` to return a boolean? I don't see any consumers using this.

::: mobile/android/chrome/content/browser.js:6755
(Diff revision 4)
>            for (let i = 2; Services.search.getEngineByName(name); i++)
>              name = title.value + " " + i;
>  
>            Services.search.addEngineWithDetails(name, favicon, null, null, method, formURL);
> -          Snackbars.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [name], 1), Snackbars.LENGTH_LONG);
> +          Snackbars.show(Strings.browser.formatStringFromName(
> +            "alertSearchEngineAddedToast", [name], 1), Snackbars.LENGTH_LONG);

Nit: We don't mind long lines in JS, this doesn't need to change.

::: mobile/android/chrome/content/browser.js:6761
(Diff revision 4)
> +
>            let engine = Services.search.getEngineByName(name);
>            engine.wrappedJSObject._queryCharset = charset;
> -          for (let i = 0; i < formData.length; ++i) {
> -            let param = formData[i];
> -            if (param.name && param.value)
> +          formData.forEach(param => { engine.addParam(param.name, param.value, null); });
> +
> +          return (resultCallback) ? resultCallback(true) : true;

Why do you remove this check for `param.name` and `param.value`?

Comment 21

[:Margaret Leibovic]

Cc'ing mkaply, just as an FYI.

Comment 22

[Mark Capella [:capella]]

@margaret: The check for `param.name` and `param.value` have been incorporated into the generating code:
_getSortedFormData(). It's that same bit of tight code for which we just expanded the sort logic.

Comment 23

[Mark Capella [:capella]]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45293/diff/4-5/

Comment 24

[:Margaret Leibovic]

Comment on attachment 8739493 [details]
MozReview Request: Bug 1262916 - Option to add custom search engine should disappear when site is added, r=margaret

https://reviewboard.mozilla.org/r/45293/#review49109

::: mobile/android/chrome/content/browser.js:6752
(Diff revision 5)
> +    return Services.search.hasEngineWithURL(method, formURL, formData);
> +  },
> +
> +  /**
> +   * Adds a new search engine to the BrowserSearchService, based on its provided element. Prompts for an engine
> +   * name, and post-pends a simple version-number in case of collision with an existing name.

s/post-pends/appends

::: mobile/android/chrome/content/browser.js:6756
(Diff revision 5)
> +   * Adds a new search engine to the BrowserSearchService, based on its provided element. Prompts for an engine
> +   * name, and post-pends a simple version-number in case of collision with an existing name.
> +   *
> +   * @return callback to handle success value. Currently used for ActionBarHandler.js and UI updates.
> +   */
> +  addEngine: function addEngine(aElement, resultCallback) {

Maybe instead of a `resultCallback` parameter that takes a boolean, you should just have a success callback, since you only actually care about executing this in the case that there is success.

::: mobile/android/chrome/content/browser.js:6786
(Diff revision 5)
> +
>      Services.search.init(function addEngine_cb(rv) {
>        if (!Components.isSuccessCode(rv)) {
>          Cu.reportError("Could not initialize search service, bailing out.");
> +        if (resultCallback) {
> +          return resultCallback(false);

You don't need the return statement here, you can just call the callback then fall down to the return statement below.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c67242e935ee

Comment 26

[Phil Ringnalda (:philor)]

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

Comment 27

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 49.0a1 (2016-05-17)

After adding imdb as a search engine, the action bar does not disappear.
It remains on screen, but the "add search engine" icon disappears from the action bar.
Is this expected?

Comment 28

[Mark Capella [:capella]]

Yes. The final solution is to remove the option if not available.

If we simply hid the actionbar after adding the search engine, then long-tapping into the field would have shown the option again.

Comment 29

[Sorina Florean [:sflorean]]

I will mark this verified based on comment #27 and #28.