Closed Bug 392143 Opened 17 years ago Closed 16 years ago

show keywords as url bar autocomplete choices

Categories

(Firefox :: Address Bar, enhancement, P2)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 3.1a1

People

(Reporter: moco, Assigned: Mardak)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [parity-opera][fixes bug 249468])

Attachments

(12 files, 18 obsolete files)

35.43 KB, image/png
beltzner
: ui-review-
Details
46.49 KB, image/png
Details
48.36 KB, image/png
Details
20.74 KB, image/png
Details
33.29 KB, image/png
Details
92.24 KB, image/png
Details
93.81 KB, image/png
Details
69.73 KB, image/png
Details
69.71 KB, image/png
Details
84.44 KB, image/png
Details
19.09 KB, image/png
Details
12.61 KB, patch
Details | Diff | Splinter Review
show keywords as url bar autocomplete choices

say I have some keywords (g for google) or (b for bugzilla).

if I type "g firefox", on the trunk and even after the fix for bug #389491 we will not show any results.

what about, if you type "g xxxx", we show "g xxxx" to indicate that it will do a keyword search, and show the keyword description as the title, and use the favicon (if we have it)

if "g xxxx" happens to be a page title or in a url, we'd still show that.

keywords should probably come first in our results.

(fwiw, opera has this.)
dupe of bug 249468
It sounds a dup of that bug based on the summary, but it's not.

See also bug 389903, which suggests showing previously used keyword searches, so if I've typed "g firefox" before, it will show up in autocomplete when I type "g fi".  I think fixing bug 389903 would be more useful.  What's the point of having an autocomplete result that's the same as pressing enter?
Whiteboard: parity-opera
> bug 389903

thanks for that reference.  that sounds useful, too.

> What's the point of having an autocomplete result that's the same as pressing enter?

it could helps the user remember my keywords, even if history has been cleared.
> it could helps the user remember my keywords, even if history has been cleared.

The scenario you described in comment 0 (type "g xxxx" into the address bar to see "g xxxx" in the autocomplete menu) wouldn't help users remember keywords.  Did you mean type "xxxx" to get "g xxxx"?  Opera does that, too.
I think makes sense to display the favicon and domain name of the search associated with the keyword, but this isn't a very high priority change.

How do keyword searches integrate with the latest changes to the location bar auto-complete?  Do they complete override all results, or simply show up first?
Should this be added to the meta bug 405745 '[meta] improve url bar autocomplete results'?
yes, thanks.
Blocks: 405745
Attached image screenshot of v1
Attached patch v1 (obsolete) — Splinter Review
Make the first item the keyword and make it selectable, so the user can either just press enter or down enter if that's more useful to the user.

Additionally, users can now have multiple searches on the same keyword and select it from the list.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #303886 - Flags: review?
Oh, and the patch is on the stack after the one for bug 395739.
Depends on: awesomebar
See also bug 418095: similar intent, but considering the new autocomplete with tagging system.
Comment on attachment 303884 [details]
screenshot of v1

FYI, I didn't have the first result in my history. I had a bookmark titled "Mozilla Identifier Search" with url "http://mxr.mozilla.org/seamonkey/ident?i=%s"

If you actually visit the page, the title would be "seamonkey identifier search "AutoCompleteProcessSearch"

So the patch uses the bookmark title, emdash, user's input for the title and replaces the %s with the search term for the url.
Comment on attachment 303884 [details]
screenshot of v1

The keyword result shows the keyword bookmark's title then emdash then whatever the user typed in, so s/he can realize the title/url is based on whatever inputs.

The url also updates and gets styled assuming bug 415403 gets fixed.
Attachment #303884 - Flags: ui-review?(beltzner)
Comment on attachment 303884 [details]
screenshot of v1

I think what we want is:

@ <titlename>: <parameters>        *
  <url-being-generated>

I think we also want to *not* show any highlights in the keyword match (unless that's seriously hard to do) since we're not actually matching against anything here, keyword doesn't necessarily match the title.
Attachment #303884 - Flags: ui-review?(beltzner) → ui-review-
Basically use ": " instead of " <emdash> " ? The highlighting thing would be tricky as the emphasis is done separately from the search and it just highlights everything even if that's not what the backend matched.
About emphasizing stuff, it's kinda a cue that it's a special result because it keeps updating and emphasizing what you're typing. So if you're curious, you see how the url is forming; otherwise you see the title updating as well.
Attached patch v1.1 (obsolete) — Splinter Review
Unbitrot and use ": " instead of " mdash "
Attachment #303886 - Attachment is obsolete: true
Attachment #306465 - Flags: review?
Attachment #303886 - Flags: review?
Hehe. I just realized the display can easily show a "keyword" tag just by replacing NULL in the query with the string 'keyword' and it'll look like

@ Title: terms    <=| keyword *
  url updating

But I suppose that would be confusing when the user tries to search for keyword...
Oh duh. It wouldn't be too hard to tell the UI it's a keyword. Just like we tell the UI it's a "favicon" (no icon), "bookmark" (star), "tag" (tag + star). Could conditionally not emphasize assuming we don't want it.
(In reply to comment #16)
> About emphasizing stuff, it's kinda a cue that it's a special result because it
> keeps updating and emphasizing what you're typing. So if you're curious, you
> see how the url is forming; otherwise you see the title updating as well.

I actually think that by *not* emphasizing the text, you get the indication that it's a special result. I'd be fine with emphasizing the %s portion, which is the dynamic bit of information that the user is interested in. That way emphasis always matches search criteria, and non-emphasis is context (in this case, the name of the keyword search).

Another possibility would be to do:

@ [<keyword>] <bookmark name> : <%s>                     *
  <url>

and put emphasis anywhere it matched. Though I think I like my first proposal better.

(In reply to comment #19)
> Oh duh. It wouldn't be too hard to tell the UI it's a keyword. Just like we
> tell the UI it's a "favicon" (no icon), "bookmark" (star), "tag" (tag + star).
> Could conditionally not emphasize assuming we don't want it.

Yeah, I was thinking you'd just put the "<bookmark name> :" part in a different CSS class.
There's 2 attributes to consider for displaying the title and url.. is the bookmark/keyword a simple page or a %s search. And did the user type in parameters or just the keyword?

search + parameters: "Title: <params>", http://site/?thing=<params>
search + keyword: "Title: <keyword>", http://site/?thing=<keyword>
simple + parameters: "Title: <params>", http://site/
simple + keyword: "Title: <keyword>", http://site/

The thing about search + keyword is that it won't be sending the http://site/?thing=%s url to the server if you select the entry.
Attached patch v2 (obsolete) — Splinter Review
Allow for a separate icon for keywords, but for now use the same as bookmark. Don't show the keyword unless it's the only thing typed; otherwise show parameters.

This patch is after bug 415403 on my stack.
Attachment #306465 - Attachment is obsolete: true
Attachment #306599 - Flags: review?
Attachment #306465 - Flags: review?
Depends on: 415403
Attached patch v2.1 (obsolete) — Splinter Review
Thought of a simpler way to not highlight keyword results.
Attachment #306599 - Attachment is obsolete: true
Attachment #306702 - Flags: review?
Attachment #306599 - Flags: review?
Patch context needs the one from bug 418257.
Depends on: 418257
You need to request review from a specific person.
beltzner: What should be shown if the generated keyword search is the same as a url in the history? Because we only show a url once, the regular search matching it doesn't show up after the keyword one is in the list.

E.g., "bug 392143" will have the first entry be..

Bugzilla: 392143
https://bugzilla.mozilla.org/show_bug.cgi?id=392143

But not show..

Bug 392143 - show keywords as url bar autocomplete choices
https://bugzilla.mozilla.org/show_bug.cgi?id=392143
That's a pretty special case, I think, but in general I think that if we have a case where the keyword-search generated URL matches an item in the user's history 1:1 then we should show the history result and not the keyword search.

The more I look at the screenshots, the more I think we need to have some special indication that it's a keyword search instead of a history result to disambiguate. I wonder if we want to encase the Title: <params> in [braces]?
Attached patch v3 (obsolete) — Splinter Review
Changed the query to look for existing pages and show those if we find one. Also, fixes a potential issue with escaped/unescaped urls mixing together.
Attachment #306702 - Attachment is obsolete: true
Attachment #307418 - Flags: review?
Attachment #306702 - Flags: review?
Attached image screenshot of v3
Blocks: 249468
Attachment #307419 - Flags: ui-review?(beltzner)
Attached image screenshot of v3.1
Some reason the query.png icon on OS X is a timer.... pretend it was something more along the lines of winstripe's.

http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/places/query.png

This avoids RTL issues with "] <params> :<title> ["
Attachment #307525 - Flags: ui-review?(beltzner)
Attached patch v3.1 (obsolete) — Splinter Review
Use icons instead of text to distinguish the keyword search. Also this makes the front-end add on the query parameters instead of having the backend do it.
Attachment #307526 - Flags: review?
Instead of adding an icon to the middle of the text, we could replace the favicon shown on the left. But there would still need to be something separating the title and params.. hopefully RTL friendly. Maybe a ndash?
ndash works, though I don't know why that's any more RTL friendly than a :
Flags: wanted-firefox3+
Comment on attachment 307526 [details] [diff] [review]
v3.1

keyword search should be case sensitive..
Attachment #307526 - Flags: review? → review-
Attached patch v3.2 (obsolete) — Splinter Review
Now with more correct unescaping for queries that keep the correct case. Spaces become + and + become %2B.
Attachment #307418 - Attachment is obsolete: true
Attachment #307526 - Attachment is obsolete: true
Attachment #307959 - Flags: review?
Attachment #307418 - Flags: review?
Comment on attachment 307959 [details] [diff] [review]
v3.2

I think you diffed wrong; this patch doesn't include the tests you added for this. :)
Attached patch v3.3 (obsolete) — Splinter Review
(In reply to comment #37)
> (From update of attachment 307959 [details] [diff] [review])
> I think you diffed wrong; this patch doesn't include the tests you added for
> this. :)
Right you are!

Here's the test that checks for regular keyword query, query with multiple params (spaces -> +), query with + (+ -> %2B), query with non-ascii (escaped), query that matches a page (show page title instead of keyword).

Bug 249468 has the test to check multiple keywords are shown.
Attachment #307959 - Attachment is obsolete: true
Attachment #307996 - Flags: review?
Attachment #307959 - Flags: review?
Attached patch v3.4 (obsolete) — Splinter Review
Unbitrot from bug 421412 and clean up the testcase so it's easier to copy for future testcases.

Also, it still isn't entirely clear what the UI should be. Right now it's

<favicon> Title <queryicon> params       <star>

Potentially we could use a special icon instead of the favicon and just do..

<keywordicon> Title -- params     <star>
Attachment #307996 - Attachment is obsolete: true
Attachment #308270 - Flags: review?
Attachment #307996 - Flags: review?
Attachment #307419 - Flags: ui-review?(beltzner)
Attachment #307525 - Flags: ui-review?(beltzner)
Question, what is the behavior supposed to be with the tags? I assume this is related.

Basically when I type a tag, they are not show together. Should this not be the case and also should they not be shown as the first result?
(In reply to comment #40)
> Question, what is the behavior supposed to be with the tags? I assume this is
> related.

No, I don't believe this bug has any to do with tags

> Basically when I type a tag, they are not show together. Should this not be the
> case and also should they not be shown as the first result?

No, I don't believe that is the intended behaviour. However, you might be interested in Bug 395161 - make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries

Blocks: 422490
No longer blocks: 422490
Depends on: 422869
(In reply to comment #39)
> Created an attachment (id=308270) [details]
> v3.4
> 
> Unbitrot from bug 421412 and clean up the testcase so it's easier to copy for
> future testcases.
> 
> Also, it still isn't entirely clear what the UI should be. Right now it's
> 
> <favicon> Title <queryicon> params       <star>
> 
> Potentially we could use a special icon instead of the favicon and just do..
> 
> <keywordicon> Title -- params     <star>
> 

If bookmarks and tags have an icon to show where the results are coming from, it would seem that the right thing is to replace that icon with one for keywords. 

Then, whichever icon is shown indicates where the match was made, with an ascending hierarchy of specificity (essentially as it is now, just with keywords usurping tags' place at the top):
 - keywords first as there's only one exact match for each of them
 - tags next as they were chosen to apply to each tagged link
 - bookmarks as the user has given them a relevance in bookmarking them
 - history finally (no icon)
FYI: tagged results will have a star icon with an extra tag icon. See attachment 307638 [details]

So keywords will probably have the star icon and potentially a keyword icon replacing the favicon if we can find a keyword icon.
Why should it replace the favicon, though? The favicon is there to instantly identify the site. 

Couldn't this keyword icon either replace the tags icon (in the same style -- [icon] keyword match) or fall below it on the second line (again -- [icon] keyword match) trimming any URLs. Trimming URLs already happens for long ones and would have little extra impact as a keyword is likely to be a short one for ease and brevity of typing. 
(This way, the keyword could still match as a tag does, and any search parameters could be highlighted in the URL as suggested above, showing attribution for all terms.)
(In reply to comment #44)
> Why should it replace the favicon, though? The favicon is there to instantly
> identify the site. 
There aren't favicons for keywords actually. It's because the page doesn't really exist on the server. "http://site/?query=%s" So it shows up as the default page icon.
Attached patch v3.5 (obsolete) — Splinter Review
Use ndash instead of an image, unbitrot from bug 422491.
Attachment #308270 - Attachment is obsolete: true
Attachment #309462 - Flags: review?
Attachment #308270 - Flags: review?
Attached image screenshot of v3.5
Multiple bookmarks with the same keyword and display.
(In reply to comment #45)
> (In reply to comment #44)
> > Why should it replace the favicon, though? The favicon is there to instantly
> > identify the site. 
> There aren't favicons for keywords actually. It's because the page doesn't
> really exist on the server. "http://site/?query=%s" So it shows up as the
> default page icon.
> 

I might be getting confused now - and sorry if so - but Firefox has to resolve
the keyword to a site/bookmark, which could have a favicon, no? And wouldn't a
static bookmark with a keyword (and no parameters to swap out in the URL) still
be able to posses a favicon like any other bookmark?
Instead of the star.. chrome://mozapps/skin/passwordmgr/key.png
(In reply to comment #49)

Does the key metaphor localise well? That is, is a key suitable to represent “keyword” in languages other than English?
Right. That's why I didn't use the key before. ;) But then I thought about it a little more.. So what does the word keyword mean? It's a key to a code.. something that unlocks.. a search! yeah ;)
Well... I don't know about any other languages but the word is "nyckelord" in swedish which literally means "keyword".
Can you include the text in the location bar in a screenshot so I can be totally clear I understand how this UI is working.  In terms of the key icon, we like to have a 1:1 ratio between physical metaphors and features in Firefox.  (For instance, we cut the filmstrip icon for plugins since we also use it to reference media on the page in Page Info).  Since these are searches, your metaphorical weapon of choice is search-glass.png :)
(In reply to comment #52)
> Well... I don't know about any other languages but the word is "nyckelord" in
> swedish which literally means "keyword".
> 

In my native French, "mot-clé" (where mot=word and clé=key) is certainly a valid translation, maybe the best one. Let me check my German dictionary... Schlüsselwort (where Schlüssel=key, Wort=word), OK.

Esperanto... I think "ŝlosilvorto" (from ŝlosilo "key", wfw. "lock-tool" + vorto "word") would be understood.

Russian... My dictionary doesn't give any translation that I can find: I would try ключевое слово (where ключевый, -ая, -ое is the adjective derived from "ключ" key; слово=word), but I've no idea if it would be understood.

What about Chinese?
(In reply to comment #54)
> Created an attachment (id=309589) [details]
> screenshot of v3.5 with search
> 

I like that metaphor, as the easiest way to create such a bookmark is the "Add a keyword for this search" context menu item in search form fields.

But that leads my train of thought to the keywords of search bar engines. Couldn't these be listed in the same way then?
Attached patch v3.6 (obsolete) — Splinter Review
Update from changes to bug 418257. Also made it a colon after the title instead of ndash. Also update the testcase to use the same template from bug 422869.
Attachment #309462 - Attachment is obsolete: true
Attachment #309712 - Flags: review?
Attachment #309462 - Flags: review?
Attached image screenshot of v3.6
Oh yeah, also switched to the search magnifying glass.
If you want to try it out:

https://build.mozilla.org/tryserver-builds/2008-03-15_16:00-edward.lee@engineering.uiuc.edu-camelBoundary.commaTags.colonKey/

Bug 393678 - location bar autocomplete should take word boundaries in account
Bug 407946 - emphasize all matching text in title and url, not just the first match in title and url
Bug 415403 - Show matches for all search words for location bar autocomplete
Bug 418257 - Show what part of which tags match for urlbar autocomplete
Bug 392143 - show keywords as url bar autocomplete choices
Bug 249468 - Add all bookmark keywords to location bar autocomplete drop-down list
Bug 395161 - make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries
Bug 407204 - adjust the title and url text sizes
Bug 406257 - reduce the number of rows in url bar autocomplete from 10 to 6
Bug 420437 - Search and emphasize quoted strings with spaces
Bug 414326 - Use DownloadUtils for software update downloads
Bug 422491 - Optimize AwesomeBar if it finished searching and found fewer than maxResults
Attached image screenshot of v3.7
ooh. icons. ;)
Attached patch v3.7 (obsolete) — Splinter Review
Guess the favicon by using the favicon of the most frecent visit to a page that shares the same host.
Attachment #309712 - Attachment is obsolete: true
Attachment #309716 - Flags: review?
Attachment #309712 - Flags: review?
Edward, one question to your attachment 309713 [details]. Do you have the keyword "mxr" set to multiple locations or why you have listed it three times with different URLs at the top? Shouldn't it be one match? Or is it because we support multiple keywords with the same name? Then it would be cool.
Right. This fixes bug 249468 as well, so we can /show/ bookmarks with the same keyword. We've always supported multiple bookmarks having the same keyword, but I don't think there was any good way to let the user choose.

Just like attachment 309715 [details], I set all the keywords to "key" and I see the whole list of them. I suppose potentially you could create a keyword "." or something for easy access to listing each search.
(In reply to comment #63)
> keyword. We've always supported multiple bookmarks having the same keyword, but
> I don't think there was any good way to let the user choose.

IMO there was no way to choose between the different bookmarks. That's why this patch rocks! It will be one of the most important features of the awesome bar (at least for me).
FYI, this will need both dietrich and gavin's reviews, but this isn't a
blocker, so I doubt requesting review now will matter much until the other
reviews are cleared.
Whiteboard: parity-opera → [parity-opera][has patch][need dietrich review][need gavin review]
Whiteboard: [parity-opera][has patch][need dietrich review][need gavin review] → [parity-opera][has patch][need dietrich review][need gavin review][need bug 418257]
Is it possible now to at least request the reviews, as this is wanted-firefox3+?
Attached patch v3.8 (obsolete) — Splinter Review
Requesting reviews, but the patch needs some other patches like bug 418257.
Attachment #309716 - Attachment is obsolete: true
Attachment #313298 - Flags: review?(gavin.sharp)
Attachment #313298 - Flags: review?(dietrich)
Attachment #309716 - Flags: review?
Edward, all bugs on the depends list are fixed now. Does your patch rely on any other patches or is it ready for review?
Attached patch v3.9 (obsolete) — Splinter Review
Unbitrot context for review.
Attachment #313298 - Attachment is obsolete: true
Attachment #315245 - Flags: review?(gavin.sharp)
Attachment #315245 - Flags: review?(dietrich)
Attachment #313298 - Flags: review?(gavin.sharp)
Attachment #313298 - Flags: review?(dietrich)
For those who have been using tryserver builds of this, has keyword search been working okay?
Flags: blocking-firefox3?
Whiteboard: [parity-opera][has patch][need dietrich review][need gavin review][need bug 418257] → [parity-opera][has patch][need dietrich review][need gavin review]
Target Milestone: --- → Firefox 3
Edward, you talk about the tryserver build mentioned in comment 59?
I'll start up some new builds, but here's one from a few days back..

https://build.mozilla.org/tryserver-builds/2008-04-09_23:10-edward.lee@engineering.uiuc.edu-no.flash/
(In reply to comment #70)
> For those who have been using tryserver builds of this, has keyword search been
> working okay?
> 

Using the Windows ZIP build from
https://build.mozilla.org/tryserver-builds/2008-04-12_01:47-edward.lee@engineering.uiuc.edu-keywords/
I'm very pleased with the interaction. I changed my bookmarks to share keywords, so that a list of keyword searches is presented, and I can select the search page of choice.

One nit I'd like to mention is the position of the "search glass" icon on the right side of the autocomplete listbox. While consistent with star and tag icons, I think it's too subtle--or disconnected from the search terms if you want.
Attached patch v3.10 (obsolete) — Splinter Review
Just updating the testcase to be much shorter now that it shares the test logic with all other places autocomplete testscases.

As pointed out, there's a build with this patch:
https://build.mozilla.org/tryserver-builds/2008-04-12_01:47-edward.lee@engineering.uiuc.edu-keywords/
Attachment #315245 - Attachment is obsolete: true
Attachment #315290 - Flags: review?(gavin.sharp)
Attachment #315290 - Flags: review?(dietrich)
Attachment #315245 - Flags: review?(gavin.sharp)
Attachment #315245 - Flags: review?(dietrich)
Depends on: 428829
(In reply to comment #70)
> For those who have been using tryserver builds of this, has keyword search been
> working okay?
> 

I have been using the mac build edward.lee@engineering.uiuc.edu-awesome.smallNfast.bar-firefox-try-mac.dmg for a couple of weeks
instead of the nightlies because of this patch.  It works great.
(In reply to comment #74)
> https://build.mozilla.org/tryserver-builds/2008-04-12_01:47-edward.lee@engineering.uiuc.edu-keywords/

A problem: after going to a suggestion two times, it starts appearing starred in the list.
That only happens for the keyword entry. If you search not using the keyword, it won't have the star.
Right, it is not actually starred. But that is still a problem, because it only happens after the suggestion is visited twice. Why?
It shows a star instead of the keyword search icon when it matches a page in your history. You don't even need to use the keyword search beforehand.
Really nice to have, absolutely not blocking, and probably too late on general principle, we're already past our code freeze date by a week.
Flags: blocking-firefox3? → blocking-firefox3-
Some people ended up using keywords as single-entry tags, not just searches. I think it would be cool if we detected when there was a %s in the keyword and used the search icon in that case, and if not, basically treated it more like a tag and just show the star icon.
Attached patch v3.11 backend (obsolete) — Splinter Review
Exact same thing as v3.10, but splitting up the patches so maybe it's easier to review.
Attachment #315290 - Attachment is obsolete: true
Attachment #316029 - Flags: review?(dietrich)
Attachment #315290 - Flags: review?(gavin.sharp)
Attachment #315290 - Flags: review?(dietrich)
Attached patch v3.11 frontend (obsolete) — Splinter Review
Attachment #316030 - Flags: review?(gavin.sharp)
Attached patch v3.11 testcase (obsolete) — Splinter Review
(In reply to comment #80)
Does that mean this feature won't have a chance to appear in 3.0, or is it just too late for RC1?

If this patch doesn't make it to the final release, any chance this could be turned into an extension, Edward? This is just too useful to leave behind!
Looking at the progress over the last 3 months, it would seem unlikely. But I'll remain optimistic... technically we haven't frozen yet. ;)

I probably won't write an extension, and if I did, the code would probably be a lot more complex than the patches I've got here. But I might just make a special tryserver build for those who want it.

The extension code would probably involve hacking around to insert results directly into the autocomplete or sit somewhere in the middle between back-end results and front-end display. But either way will probably slow things down for the common case of just doing normal page searches.
I've added an add-on that provides similar functionality as what would be provided by the patches in this bug.

https://addons.mozilla.org/en-US/firefox/addon/7755/
Priority: -- → P2
Target Milestone: Firefox 3 → Firefox 3.1
Comment on attachment 316029 [details] [diff] [review]
v3.11 backend


>@@ -711,16 +765,27 @@ nsNavHistory::AutoCompleteProcessSearch(
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       // Always prefer the bookmark title unless it's empty
>       nsAutoString title =
>         entryBookmarkTitle.IsEmpty() ? entryTitle : entryBookmarkTitle;
> 
>       nsString style;
>       switch (aType) {
>+        case QUERY_KEYWORD: {
>+          // If we don't have a title, then we must have keyword, so let the UI

mega-nit: "have a keyword"

>@@ -753,17 +818,18 @@ nsNavHistory::AutoCompleteProcessSearch(
>       PRBool showTags = !entryTags.IsEmpty();
> 
>       // Add the tags to the title if necessary
>       if (showTags)
>         title += kTitleTagsSeparator + entryTags;
> 
>       // Tags have a special style to show a tag icon; otherwise, style the
>       // bookmarks that aren't feed items and feed URIs as bookmark
>-      style = showTags ? NS_LITERAL_STRING("tag") : (parentId &&
>+      style = !style.IsEmpty() ? style :
>+        showTags ? NS_LITERAL_STRING("tag") : (parentId &&
>         !mLivemarkFeedItemIds.Get(parentId, &dummy)) ||
>         mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ?
>         NS_LITERAL_STRING("bookmark") : NS_LITERAL_STRING("favicon");

chaining ternary expressions like this makes the code more difficult to read. the same result could be achieved in a clearer manner with:

if (style.IsEmpty()) {
  ...
}
Attachment #316029 - Flags: review?(dietrich) → review+
Whiteboard: [parity-opera][has patch][need dietrich review][need gavin review] → [parity-opera][has patch][need gavin review]
Flags: wanted-firefox3.1?
Comment on attachment 316030 [details] [diff] [review]
v3.11 frontend

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>+            let params = search.substr(search.search(' ') + 1);

use indexOf instead?

The hardoded ":" and element/string concatenation kind of bother me, but I can't really pinpoint why, and I suppose there aren't really any other options here short of reworking autocomplete APIs.
Attachment #316030 - Flags: review?(gavin.sharp) → review+
Attached patch v3.12Splinter Review
(In reply to comment #89)
> >+          // If we don't have a title, then we must have keyword, so let the UI
> mega-nit: "have a keyword"
Fixed.

> >@@ -753,17 +818,18 @@ nsNavHistory::AutoCompleteProcessSearch(
> >-      style = showTags ? NS_LITERAL_STRING("tag") : (parentId &&
> >+      style = !style.IsEmpty() ? style :
> >+        showTags ? NS_LITERAL_STRING("tag") : (parentId &&
> >         !mLivemarkFeedItemIds.Get(parentId, &dummy)) ||
> >         mLivemarkFeedURIs.Get(escapedEntryURL, &dummy) ?
> >         NS_LITERAL_STRING("bookmark") : NS_LITERAL_STRING("favicon");
> chaining ternary expressions like this makes the code more difficult to read.
> the same result could be achieved in a clearer manner with:
Changed to double nested if/if,elseif,else statement.

(In reply to comment #90)
> >+            let params = search.substr(search.search(' ') + 1);
> use indexOf instead?
Sure.

> The hardoded ":" and element/string concatenation kind of bother me, but I
> can't really pinpoint why
RTL/LTR wouldn't be an issue with ":", and it's acting like the existing tag icon as a separator, except it's a character instead of an image.
Attachment #316029 - Attachment is obsolete: true
Attachment #316030 - Attachment is obsolete: true
Attachment #316032 - Attachment is obsolete: true
Flags: in-testsuite?
Flags: in-litmus?
Flags: in-litmus-
Whiteboard: [parity-opera][has patch][need gavin review] → [parity-opera][has patch]
http://hg.mozilla.org/mozilla-central/index.cgi/rev/a15164388bf1
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [parity-opera][has patch] → [parity-opera][fixes bug 249468]
Target Milestone: Firefox 3.1 → Firefox 3.1a1
Does this obsolete the Show Keywords addon, or does that still do things not included here?
Show Keywords add-on is obsolete for 3.1 with keyworded bookmarks, but I did happen to add support for searchbar keyworded search engines to the add-on in a later version. 

Filed Bug 445955 – Put search bar keyworded searches in location bar autocomplete
Verified with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071903 Minefield/3.1a1pre ID:2008071903

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008071806 Minefield/3.1a1pre ID:2008071806
Status: RESOLVED → VERIFIED
Blocks: 448033
Flags: wanted-firefox3.1? → wanted-firefox3.1+
Depends on: 453120
(In reply to comment #79)
> It shows a star instead of the keyword search icon when it matches a page in
> your history. You don't even need to use the keyword search beforehand.

Filed bug 453246 for that.

---

Also, I have a deviantArt search keyword, "d", but I also type d to match digg.com. With this behavior, digg is always in the second spot instead of the first and I have to press down twice to get to it instead of once. Also, when searching deviantArt, pressing down + enter is the same as just pressing enter. Is there another benefit to selecting the keyword entry in the dropdown instead of just pressing enter?
(In reply to comment #97)
> Also, I have a deviantArt search keyword, "d", but I also type d to match
> digg.com. With this behavior, digg is always in the second spot instead of the
> first and I have to press down twice to get to it instead of once. Also, when
> searching deviantArt, pressing down + enter is the same as just pressing enter.
> Is there another benefit to selecting the keyword entry in the dropdown instead
> of just pressing enter?

One possible choice would be to only include the keyword result when the user presses space after d, and only if d is at the beginning of the text, i.e., when what the user has typed in the location bar is /^d\s.*$/.

Of course I guess you should file a new bug for this.
Depends on: 454208
Blocks: 455561
Blocks: 463009
You need to log in before you can comment on or make changes to this bug.