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

VERIFIED FIXED in Firefox 36

Status

()

Firefox
Location Bar
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Summary: Selection a moz-action result and typing more can result in "Search X for moz-action:..." item → Selecting a moz-action: result then typing more can result in "Search X for moz-action:..." item
Well, this turned out to be "fun". And blocking bug 1067903.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
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.
Iteration: --- → 35.2
Flags: needinfo?(mmucci)

Updated

3 years ago
Blocks: 1067903
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

Updated

3 years ago
Blocks: 1038614

Updated

3 years ago
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
Created attachment 8495934 [details] [diff] [review]
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.
Attachment #8495934 - Flags: review?(mak77)
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?
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(bmcbride)
Created attachment 8496509 [details] [diff] [review]
Patch v2
Attachment #8495934 - Attachment is obsolete: true
Attachment #8495934 - Flags: review?(mak77)
Attachment #8496509 - Flags: review?(mak77)
Comment on attachment 8496509 [details] [diff] [review]
Patch v2

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

lgtm
Attachment #8496509 - Flags: review?(mak77) → review+
 https://hg.mozilla.org/integration/fx-team/rev/262b6ad34702
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
https://hg.mozilla.org/integration/fx-team/rev/c5f1d81d3849
Backed out in https://hg.mozilla.org/integration/fx-team/rev/29854f8b8dd9, hello from bc1!

Updated

3 years ago
Iteration: 35.2 → 35.3
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.

Updated

3 years ago
No longer blocks: 1067903
Depends on: 1067903
Comment hidden (off-topic)
Comment hidden (off-topic)
Duplicate of this bug: 1078357

Updated

3 years ago
Iteration: 35.3 → 36.1

Updated

3 years ago
Iteration: 36.1 → ---
Created attachment 8519538 [details]
MozReview Request: bz://1070778/Unfocused
/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
Created attachment 8519540 [details]
MozReview Request: bz://1070778/Unfocused
/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
Attachment #8519540 - Flags: review?(mak77)
/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
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 :(

Updated

3 years ago
Iteration: --- → 36.2
Attachment #8519538 - Attachment is obsolete: true
Attachment #8519540 - Attachment is obsolete: true
Attachment #8519540 - Flags: review?(mak77)

Updated

3 years ago
Iteration: 36.2 → 36.3
Blocks: 995091
Blocks: 1099046
https://hg.mozilla.org/integration/fx-team/rev/2acc754928c8
No longer blocks: 1099046
https://hg.mozilla.org/mozilla-central/rev/2acc754928c8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.