Location bar doesn't use google 'I'm feeling lucky' if the first word typed is a keyword (breaking change)

RESOLVED FIXED in Firefox 3 beta5

Status

()

Firefox
Address Bar
P2
major
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Darren, Assigned: Gavin)

Tracking

({regression})

Trunk
Firefox 3 beta5
regression
Points:
---
Bug Flags:
blocking-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

---
I've marked this major because the "I'm feeling lucky" auto-search is IMHO, a major feature and the current behavior departs from previous versions of FF.
---

See steps to reproduce, which should be self-explanatory.

Reproducible: Always

Steps to Reproduce:
1. Pick two words (or more) and type them into the location bar, noting the page you come up with - should be a google search results page or a website;
2. Assign the first word to point to a page *other than* the one you end up at in step 1;
3. Type the two (or more) words into the location bar again.
Actual Results:  
You are directed to the bookmarked page.

Expected Results:  
In earlier FF versions you will still go to the google search result. This is expected because you haven't just typed the keyword, you have typed in something else that just happens to contain the keyword.

The expected results can be gotten by swapping the order of words.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030100 Minefield/3.0b4pre

When I type amo tab in the locationbar it does a search on https://addons.mozilla.org/en-US/firefox/search?q=&status=4 for the word tab.
It does this as long as I can remember, because this is the function of the keyword amo. Do you expect a google "I'm feeling lucky search" for amo tab?

(Reporter)

Comment 2

10 years ago
I wasn't explicit enough in my explanation, sorry.

The behaviour you describe is correct for keyword _searches_, but I'm talking about straight bookmarks that have a keyword assigned to them. In other words, the keyword will never take arguments (like with a keyword search) because they are meaningless.

In earlier versions of FF the location bar recognises this fact and takes a query of "keyword" and "keyword anotherkeyword" as completely separate things. The beta takes the latter term as just "keyword" and ignores the existence of the extra word.

Apologies for that explanation being so roundabout.
Confirmed with latest trunk on Windows XP. 
Regression range is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1182936900&maxdate=1182974219
Bug 378553 seems to be the most likely cause. Bug 419855 seems to have been a similar bug but has a resolution. 
Blocks: 378553
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: unspecified → Trunk
Assignee: nobody → gavin.sharp
Assignee: gavin.sharp → nobody
Created attachment 306843 [details] [diff] [review]
patch

As far as I can tell, the existing |} else| branch is useless - it would only take effect for bookmark keywords that have postData, but that don't have a replaceable string in that postData or in the URI. I don't think it's ever been possible to add such a bookmark (since the "add keyword for this search" code always passes in a %s), and even if it somehow was, sending the postData when we didn't before shouldn't really hurt anything.

So I'm modifying that branch to catch this case - a keyword that takes no parameter, but that was given one anyways. We'll just return the original "URL" in that case (really a multi-word string).

The test depends on bug 420549, since removing the engine shortly after adding causes bad stuff to happen. If that doesn't land before this patch I'll comment out the engine removal code until it does (the profile gets deleted on each unit test run anyways, I just wanted to be sure that the added engines wouldn't affect other tests).
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #306843 - Flags: review?(rflint)
Flags: blocking-firefox3?
Priority: -- → P1
Target Milestone: --- → Firefox 3
Created attachment 306859 [details] [diff] [review]
fix postData test
Attachment #306843 - Attachment is obsolete: true
Attachment #306859 - Flags: review?(rflint)
Attachment #306843 - Flags: review?(rflint)
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P1 → P2
Whiteboard: [has patch][needs review ryan]
Comment on attachment 306859 [details] [diff] [review]
fix postData test

(In reply to comment #4)
> As far as I can tell, the existing |} else| branch is useless - it would only
> take effect for bookmark keywords that have postData, but that don't have a
> replaceable string in that postData or in the URI. I don't think it's ever been
> possible to add such a bookmark (since the "add keyword for this search" code
> always passes in a %s), and even if it somehow was, sending the postData when
> we didn't before shouldn't really hurt anything.
I vaguely remember adding it in to cover that case only because I hit it while doing testing by hand. I agree that we really shouldn't hit that in practice and that it's fine to repurpose it for this.

++ for the excellent test coverage :)
Attachment #306859 - Flags: review?(rflint) → review+
Keywords: checkin-needed
Whiteboard: [has patch][needs review ryan] → [has patch]
mozilla/browser/base/content/browser.js 	1.990 	mozilla/browser/base/content/test/Makefile.in 	1.8 	mozilla/browser/base/content/test/browser_getshortcutoruri.js 	1.1 
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: Firefox 3 → Firefox 3 beta5

Updated

10 years ago
Duplicate of this bug: 419855
You need to log in before you can comment on or make changes to this bug.