1070778 - Selecting a moz-action: result then typing more can result in "Search X for moz-action:..." item

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

STR:
1. Add a bookmark with a keyword of "never "
2. Type in the urlbar "never gonna give you u"
3. See result for using the keyword search, with query string "gonna give you u"
4. If the patch from bug 1067903 is applied, select that item and re-select. Otherwise just select that item
5. Type in "p"

This will end up showing "Search X for moz-action:...", where X is your current search engine, and the moz-action: URL is the internal URL of the result seen in step 3.

The annoying thing is, I'm sure we've fixed this before.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Well, this turned out to be "fun". And blocking bug 1067903.

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 35.2.  Marking Bug 1067903 as blocked until Bug 1070778 is resolved.

(In reply to Blair McBride [:Unfocused] from comment #1)
> Well, this turned out to be "fun". And blocking bug 1067903.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I've partly figured this out.

The main problem here is that the textValue property is used as the basis for autocomplete searches. When a moz-action: result is selected, we set the value of the input - this is the moz-action: URL. We stash that away in _value, and set the display value to the input string of the action. However, textValue's getter just naively returns the full value that we use when handling the urlbar input as a command - so we end up searching for a moz-action: URL.

However, the part I haven't yet figured out is that once you've selected a moz-action: result, the first input event is effectively ignored. The second input event works fine, and performs the search correctly. So we end up with the following steps taking place:

1) Assume a bookmark with a keyword "go"
2) Type "go g" into urlbar
3) Select the keyword result
4) Type "g" again
5) Display is not updated, new search is not performed
6) Type "g" again
7) Display is updated  and new search is performed

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

That was "fun".

nsAutocompleteController uses .textValue as the basis for searches - which previously was just returning the full value, which is a moz-action: URI.

Fixing that then ended up conflicting with the onBeforeValueGet hack that was added 4.5 years ago when we first added tab-matches as a moz-action URI. Removing that, we instead get it's benefit via the more proper method of the .textValue fix above.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8495934 [details] [diff] [review]
Patch v1

Review of attachment 8495934 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_bug1070778.js
@@ +38,5 @@
> +                                         PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                         "keyword abc");
> +  itemIds.push(itemId);
> +
> +  

too many newlines and trailing spaces

::: browser/base/content/urlbarBindings.xml
@@ +717,5 @@
> +        <getter><![CDATA[
> +          let value = this.value;
> +          let action = this._parseActionUrl(value);
> +          if (action && "input" in action.params)
> +            return action.params.input;

this handles keyword actions where params include an input property... what about the other actions having .url but not .input?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> this handles keyword actions where params include an input property... what
> about the other actions having .url but not .input?

Oh, good point. Forgot that we're not enforcing that. And, in fact, the better alternative is much simpler anyway.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8496509 [details] [diff] [review]
Patch v2

Review of attachment 8496509 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bah, backed out due to bc1 failures. I swear I saw this go green on Try :\

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

Failure logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=49106207&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=49106586&tree=Fx-Team

Only results for Linux so far, but I wonder if this is Linux only yet again...
https://tbpl.mozilla.org/?tree=Fx-Team&rev=262b6ad34702

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/29854f8b8dd9, hello from bc1!

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note: This is blocked on a test regression, which was awkward to fix in any other place than bug 1067903. So landing this bug is blocked on landing that bug.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

I just hit something similar and I'm not sure whether to file a new bug or that would be fixed by this one...

STR:

1) Type "asdf" in a new tab (something which would make search the default action)
2) Hit the "arrow down" key to select the result
3) Hit "backspace" to delete the last character from "asdf"

It now shows:

Visit: {"url":"https://www.google.com/search?q=moz-action%3Asearchengine%2C{"engineName" ...

in the url bar. Should I file a new issue or is that the same?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Argh, further down in the etherpad it was. Bug 1078357. Sorry for the spam.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: MozReview Request: bz://1070778/Unfocused

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

/r/329 - Bug 1070778 - Selecting a moz-action: result then typing more can result in "Search X for moz-action:..." item. r=mak
/r/331 - Bug 1067903 - Part 1: Autoselect first autocomplete result.
/r/333 - Bug 1067903 - Part 2: Change how we handle the real value vs the display value in the URLbar
/r/335 - Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue

Pull down these commits:

hg pull review -r 3dab0efeb05043c317d188352af144eca138e389

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: MozReview Request: bz://1070778/Unfocused

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

/r/339 - Bug 1070778 - Selecting a moz-action: result then typing more can result in "Search X for moz-action:..." item. r=mak
/r/341 - Bug 1067903 - Part 1: Autoselect first autocomplete result.
/r/343 - Bug 1067903 - Part 2: Change how we handle the real value vs the display value in the URLbar
/r/345 - Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue

Pull down these commits:

hg pull review -r 3dab0efeb05043c317d188352af144eca138e389

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

/r/339 - Bug 1070778 - Selecting a moz-action: result then typing more can result in "Search X for moz-action:..." item. r=mak
/r/341 - Bug 1067903 - Part 1: Autoselect first autocomplete result.
/r/343 - Bug 1067903 - Part 2: Change how we handle the real value vs the display value in the URLbar
/r/345 - Bug 1067903 - Part 3: Update tests to deal with autoselect and textValue

Pull down these commits:

hg pull review -r 3dab0efeb05043c317d188352af144eca138e389

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ok, WTF.

Marco: reviewboard is screwing up, and I don't intend to figure it out at 2am. If you see this before I wake up: The patch for this bug hasn't changed. The patches for bug 1067903 do need review... but I suspect review comments will end up in this bug :(

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2acc754928c8

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Reproduced this issue on Windows 8 32bit, Mac OS X 10.9.5 and Ubuntu 12.04 32bit using old Nightly (2014-09-22), verified that the issue is fixed on same platforms using latest Nightly (buildID: 20141127030208).