404279 - Suggest automatic filling of 'name' field after selecting "add a keyword for this search" from context menu.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Sean]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

I suggest automatic filling on the 'name' field after selecting "add a keyword for this search" from context menu. The name could be automatically filled in based on the name of the page (the one that appears in the title bar).

Reproducible: Always

Steps to Reproduce:
1. Right click in a website text box designed for sending queries.
2. Select "add a keyword for this search".
3. Notice that the 'name' field is blank.
4. Wish that it was already filled in, based on the name of the webpage in question.
Actual Results:  
'Name' field was blank. I had to fill it in.

Expected Results:  
The 'name' field should have contained the name of the page in which the search box was encountered.

Comment 1

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

Sounds like a good idea.

Comment 2

[Tracy Walker [:tracy]]

I'm thinking many pages a user creates a search keyword bookmark for is probably also in their bookmark collection.  For example bugzilla.mozilla.org and search for a bug #. having them both automatically named the same thing is confusing. I'd prefer to name the search something unique to distinguish it from the page bookmark.

Perhaps the auto fill of "add a keyword for this search..." Name: field could be something like <page name> - keyword or <page name> search ?

Comment 3

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

(In reply to comment #2)
> Perhaps the auto fill of "add a keyword for this search..." Name: field could
> be something like <page name> - keyword or <page name> search ?

For long page titles the additional keyword or search part will not be seen while it's too much on the right side and users normally don't have such a wide sidebar or Library window. Perhaps placing "Search: " in front of the page title would be a nice option.

Comment 4

[Michael Kohler [:mkohler]]

Attachment: Patch v1

This puts "Keyword:" in front of it. Like that we don't have to localize I guess..

Comment 5

[Shawn Wilsher :sdwilsh]

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

We do in fact have to localize this because of RTL languages.

Comment 6

[Michael Kohler [:mkohler]]

Attachment: Patch v1.1 (now localizable)

Comment 7

[Michael Kohler [:mkohler]]

Attachment: Patch v1.2 (localizable + correct variable name)

Now also with the correct variable name (prefix instead of sitename).. Sorry about that.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 374706 [details] [diff] [review]
Patch v1.2 (localizable + correct variable name)

My point was that you can't assume that we always want to prefix it as it is going to be locale independent.  mak is also the appropriate reviewer when this patch is ready.

Comment 9

[Michael Kohler [:mkohler]]

Attachment: Patch v2 (fully localizable I hope)

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 374795 [details] [diff] [review]
Patch v2 (fully localizable I hope)

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -5796,18 +5796,20 @@ function AddKeywordForSearchField()
> 
>   var postData;
> 
>   if (isURLEncoded)
>     postData = formData.join("&");
>   else
>     spec += "?" + formData.join("&");
> 
>+  var title = gNavigatorBundle.getFormattedString("searchKeyword.autofilling",
>+                                                  new Array(node.ownerDocument.title));

instead of new Array simply use [node.ownerDocument.title]
also please move this in the upper part of the method where we gather other informations, should be cleaner.


>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties
>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
>@@ -199,8 +199,12 @@ safebrowsing.notAnAttackButton.accessKey
> # LOCALIZATION NOTE (privateBrowsingYesTitle, privateBrowsingNoTitle, privateBrowsingNeverAsk):
> # Access keys are specified by prefixing the desired letter with an ampersand.
> privateBrowsingDialogTitle=Start Private Browsing
> privateBrowsingMessageHeader=Would you like to start Private Browsing?
> privateBrowsingMessage=%S will save your current tabs for when you are done with your Private Browsing session.
> privateBrowsingYesTitle=&Start Private Browsing
> privateBrowsingNoTitle=&Cancel
> privateBrowsingNeverAsk=&Do not show this message again
>+
>+# Used as prefix/postfix for the autofilled keyword name when saving a search keyword
>+# LOCALIZATION NOTE (keywordSearch.autofilling): %S will be replaced by the site's title
>+searchKeyword.autofilling=Keyword: %S

call this addKeywordTitleAutoFill, i prefer the "Search: " prefix, should help non-techical users, but we can ask an ui-r on that later.
LOCALIZATION NODE should point to the same name as the property, and any eventual comment should be after it, not before.

Please on next patch ask an ui-r to faaborg or boriss for the choice of the prefix.

Comment 11

[Michael Kohler [:mkohler]]

Attachment: includes the changes from comment 10

this includes the changes from comment 10.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 376042 [details] [diff] [review]
includes the changes from comment 10

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties
>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
>@@ -199,8 +199,12 @@ safebrowsing.notAnAttackButton.accessKey
> # LOCALIZATION NOTE (privateBrowsingYesTitle, privateBrowsingNoTitle, privateBrowsingNeverAsk):
> # Access keys are specified by prefixing the desired letter with an ampersand.
> privateBrowsingDialogTitle=Start Private Browsing
> privateBrowsingMessageHeader=Would you like to start Private Browsing?
> privateBrowsingMessage=%S will save your current tabs for when you are done with your Private Browsing session.
> privateBrowsingYesTitle=&Start Private Browsing
> privateBrowsingNoTitle=&Cancel
> privateBrowsingNeverAsk=&Do not show this message again
>+
>+# LOCALIZATION NOTE (addKeywordTitleAutoFill): %S will be replaced by the site's title

"by the page's title"

>+# Used as prefix/postfix for the autofilled keyword name when saving a search keyword

i would remove "as a prefix/postfix" since this also includes title, so maybe "Used as the bookmark name when saving a keyword for a search field."

r=me with those fixed, waiting for ui-r for the prefix choice, but i personally like "Search: page title"

Comment 13

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

Comment on attachment 376042 [details] [diff] [review]
includes the changes from comment 10

After a bunch of debating on Keyword Search vs. Search, I think Search is probably fine. We should lose the : though, just:

Search CNN.com - Breaking News 

or 

Search Bugzilla@Mozilla

Comment 14

[Michael Kohler [:mkohler]]

Attachment: ready for check-in

mak's and beltzner's changes included.

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/9eace9771241

This can't make 1.9.1 due to l10n changes.

Comment 16

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

Verified fixed on trunk with builds on Windows and OS X:

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

Comment 17

[Rimas Kudelis [use needinfo to ping]]

Could anyone give a screenshot with this feature in use?

I don't quite understand how to localize it yet...

Comment 18

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

(In reply to comment #17)
> Could anyone give a screenshot with this feature in use?
> 
> I don't quite understand how to localize it yet...

The proposal is just the web page title with "Search " as prefix, e.g. "yahoo Deutschland" => "Search yahoo Deutschland".