Closed Bug 1070778 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Address Bar, defect)

Product:
Firefox
Component:
Address Bar
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 36
Iteration:
36.3

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

Attachments

(1 file, 3 obsolete files)

Patch v2
7.12 KB, patch
mak
: review+
Details | Diff | Splinter Review 
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)
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
Blocks: 1038614
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
Attached patch Patch v1 (obsolete) — — Splinter Review 
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)
Attached patch Patch v2 — — Splinter Review 

        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+
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.
No longer blocks: 1067903
Depends on: 1067903

    
    

    
      
  

    
    

    
      
  
Iteration: 35.3 → 36.1
Iteration: 36.1 → ---

    
    

    
  
/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

    
    

    
  
/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 :(
Iteration: --- → 36.2

        Attachment #8519538 -
        Attachment is obsolete: true

        Attachment #8519540 -
        Attachment is obsolete: true

        Attachment #8519540 -
        Flags: review?(mak77)
Iteration: 36.2 → 36.3
Blocks: 1099046
No longer blocks: 1099046

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2acc754928c8
Status: ASSIGNED → RESOLVED
Closed: 9 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.