498407 - Adding search keyword doesn't work on many sites

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Theodore]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1) Gecko/20090612 Firefox/3.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1) Gecko/20090612 Firefox/3.5

When I right click a search field and select "add a keyword for this search" doesn't work as expected on many sites (dictionary.com, pcworld.com, bing.com)

Reproducible: Always

Steps to Reproduce:
1. Go to http://dictionary.reference.com/ (or http://www.pcworld.com/ or http://www.bing.com/) - those are the sites I tested.
2. Right-click the search field at the top and select "Add a Keyword for this Search..."
Actual Results:  
Opens a titlebar that says "New Bookmark" You have to resize the bar to reveal the whole "New Bookmark" window, which has many fields: Name, Location, Feed location, Site location, Folder, Tags, Keyword, Description, and a "Load this bookmark in the sidebar" checkbox.
The "Folder" field doesn't let me select any of my folders. I added a name and keyword, then clicked save; but the keyword didn't work when I tried to type it into the Awesome bar. I couldn't find my keyword bookmark in the organize bookmarks window either.

Expected Results:  
I works correctly on http://www.google.com/ (opens a "New Bookmark" dialog box with 3 fields: Name, Folder, and Keyword; lets me select one of my folders, the keyword works).

Adding a search keyword for all the sites I tested works fine in Firefox 3.0.11, so this must be a bug in Firefox 3.5.
These are the addons I have installed: Gmail Notifier, Google Reader Watcher, and Personas for Firefox v. 1.2.

Comment 1

[Drew Willcoxon :adw]

Confirming on trunk and branch.  Problem does not exist on 3.0.  As noted in comment 0, Google also works correctly for me but dictionary.reference.com does not.  From error console (on trunk):

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.itemHasAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///Users/adw/trees/mozilla-central/obj-opt-shared/dist/Minefield.app/Contents/MacOS/components/nsPlacesTransactionsService.js :: placesSetItemAnnotationTransactions :: line 755"  data: no]

Comment 2

[Drew Willcoxon :adw]

In my limited testing one thing that the broken sites (http://dictionary.reference.com/, http://www.bing.com/) have in common is that they provide a description that gets picked up here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5831

The working site I tried (Google) doesn't.

It's no wonder this is breaking.  We're passing around an itemId of -1 (to signal to somebody a new item?)...

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/bookmarkProperties.js#579

... and it eventually gets passed into the annotation service:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/nsPlacesTransactionsService.js#756

Comment 3

[:Gavin Sharp [email: [e-mail]]]

regression from bug 497563?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

On OSX this expresses itself in a worse way :(

The "New Bookmark" dialog (which reporter indicated needed to be expanded) appears as a sliver beneath the actual window titlebar, and since it's a modal, it looks like the entire browser has frozen. Hitting ESC returns the browser to being responsive.

This is pretty bad. Need some time to think about whether it's bad enough to require an RC2.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #2)
> It's no wonder this is breaking.  We're passing around an itemId of -1 (to
> signal to somebody a new item?)...
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/bookmarkProperties.js#579

the -1 is perfectly valid here, this is a child transaction, since it can't know the itemId where it will be inserted it is set to -1, and replaced later by the transactions manager.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

but the transactions manager code looks wrong up there. taking.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

and, yes, bug 497563 has made this visible, previously itemHasAnnotation was always returning true or false, so passing -1 to it was returning false. But i think that the new behavior is less error prone.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 383459 [details] [diff] [review]
patch v1.0

r=me

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Let's get this on mozilla-central ASAP?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/97e34ff5d496

Comment 12

[Theodore]

Will this be fixed in Firefox 3.5 final? I need this functionality.

Comment 13

[u279076]

This might be a different bug, but how is this any different than clicking the Firefox searchbox dropdown, Manage Search Engines, selecting a search engine, Edit Keyword then adding a keyword?

There seems to be no commonality between the two (i.e. I can add a keyword of "truck" to Google Search but that is not reflected in the New Bookmark UI triggered by right clicking on the website search box.  

Should the two not be linked?  In other words, instead of creating a new bookmark with a tag, right-click-add-keyword would add the search engine to the search box list (if it did not exist already) and pop up the Manage Search Engine's Edit Keyword UI.  If this bug is about creating keywords to a search engine, I think it makes more sense this way.

Comment 14

[Dietrich Ayala (:dietrich)]

per shaver's email to release-drivers, marking this blocking. also marking in-litmus+ since there are tests there already, they just weren't run between the point at which this regressed and now.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #13)
> Should the two not be linked?  In other words, instead of creating a new
> bookmark with a tag, right-click-add-keyword would add the search engine to the
> search box list (if it did not exist already) and pop up the Manage Search
> Engine's Edit Keyword UI.  If this bug is about creating keywords to a search
> engine, I think it makes more sense this way.

Both are separated from each other. So the behavior you can see is correct. Search engines point mostly to another URL also depended on the installed locale.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with builds on all platforms like:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090616 Minefield/3.6a1pre ID:20090616111535

Comment 17

[Dietrich Ayala (:dietrich)]

1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cba3751c7311

Comment 18

[Tony Chung [:tchung]]

Adding [FFT3.5] in whiteboard.  Came across this issue during RC1 testing

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I've updated https://litmus.mozilla.org/show_test.cgi?id=6094. This testcase is now in the BFT section because it's a common way users are using the web.

Verified fixed on 1.9.1 with builds on all platforms with RC2 builds like:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 (.NET CLR 3.5.30729) ID:20090616224221

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Note: this also made impossible to set a bookmark to be opening in the sidebar ("Load in sidebar" option).
Cause is the same and touched code is the same.
I can verify that was broken on yesterday build, while it is working on today's build.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> Note: this also made impossible to set a bookmark to be opening in the sidebar
> ("Load in sidebar" option).

Via which STR? All bookmark properties dialogs or Library?

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Actually i tried that using a sidebar.addPanel link, but i suppose that any try to flag "Load in sidebar" checkbox would throw.