418257 - Show what part of which tags match for urlbar autocomplete

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Ed Lee :Mardak]

Attachment: screenshot of bug 401660 v1

Tags can be thought like extra search terms for a page in addition to the bookmark title, page title and url, so we should show what parts of the tags are being matched, especially for partial matches.

This doesn't look quite like the mockup from bug 393508, but with a text display, we can style it like the rest of the title/url as well as easily show partial matches.

Comment 1

[Ed Lee :Mardak]

Attachment: screenshot of v1

Initial tweak without any styling: Forced ()s and tag inside "(".

Perhaps margin-left of the tag hbox and padding around the tag image?

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Initial stab at tags with tag icon and bookmark star. No styling yet.

Comment 3

[Ed Lee :Mardak]

Attachment: screenshot of v1.1

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1

Comment 5

[Ed Lee :Mardak]

Attachment: screenshot of v2

Now with more spacing and correct ellipsis handling. :)

Comment 6

[Ed Lee :Mardak]

Attachment: v2

We pass the tags from the backend to the UI as "title (<tags>)" and we just parse that out in the UI to put into xul box/description/html:spans. This patch is after that of bug 415403 (and bug 407946).

Comment 7

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

Comment on attachment 305864 [details]
screenshot of v2

I'm pretty sure that this is right. We might want to tweak the tag CSS later, but that's easytimes.

Comment 8

[Ed Lee :Mardak]

Attachment: v2.1

Now with fewer spacers and silly method parameters and smaller (normal) font size for tags and hitbox for tooltip is the whole row instead of just the description element (which didn't include the "...").

Comment 9

[Ed Lee :Mardak]

Attachment: v2.2

I noticed the images were taller than usual when I tried adding a star or some other image for keyword matches. Turns out I needed to align center. Also, I noticed textContent was picking up keyword text (from a later patch).. so don't just use the parent's textContent.

Hrmm.. the ellipsis also kinda just take up space. The location bar just cuts off. Gmail has email previews that just cut off. Small width location bars would show a lot of "..." that could be used to show actual text.

Comment 10

[Ed Lee :Mardak]

Attachment: screenshot of v2.2

Icons aren't super tall anymore. Before they were 16x19 without the align center.

Comment 11

[Ed Lee :Mardak]

Attachment: v2.3

Unbitrot from bug 421412.

Comment 12

[Ed Lee :Mardak]

Attachment: v2.4

Convert the internal representation to be RTL friendly even though we don't show it in the UI. Just incase someone else wants to use it.

Comment 13

[Ed Lee :Mardak]

Right now, results in the autocomplete at best shows a tag icon for pages that are tagged, but users have no way to see what other tags are set for the page and which ones have matched.

Do we want users to tag pages and use them to find them from the location bar?

This patch would show tags for all pages and indicate what parts of them are matching.

Comment 14

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

Now that we're matching partial tags, this is pretty important for discoverability.

Comment 15

[Ed Lee :Mardak]

Attachment: v2.5

A gavin-friendly patch from the top of my queue after unbitrotting it from the process of moving it to the top of my stack.

r?dietrich for the backend r?gavin for the frontend.

Using ndash to let the frontend know what's the title and what's the tag.

Comment 16

[Dietrich Ayala (:dietrich)]

i don't see tags, and get this error with this patch:

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "aDescriptionElement.childNodes[0] is undefined" {file: "chrome://global/content/bindings/autocomplete.xml" line: 1130}]' when calling method: [nsIAutoCompleteInput::popupOpen]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

also, i know it's not intended to be displayed, but mdash might be more appropriate since ndash is intended for numerical separation.

Comment 17

[Ed Lee :Mardak]

Oh bah bah bah bah. This needs bug 407946.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 308655 [details] [diff] [review]
v2.5



>       // Add the tags to the title if necessary
>       if (showTags) {
>         // Always show the bookmark if possible when we have tags
>         useBookmark = !entryBookmarkTitle.IsEmpty();
>-        /* XXX bug 418257 to look at RTL issues of appending tags
>         (useBookmark ? entryBookmarkTitle : entryTitle)
>-          += NS_LITERAL_STRING(" (") + entryTags + NS_LITERAL_STRING(")");
>-        */
>+          += NS_LITERAL_STRING(" \u2013 ") + entryTags;
>       }

the ndash approach is hacky, i'm not liking that much. however, displaying the matching tags is worth a little unpleasantness i think. we can re-architect, or implement nsIPlacesAutoCompleteResult in the next release :P

please put the delimiter in a const, and document it's purpose. also, i'd like to see the test broken out into one specific to this bug.

r=me with these fixed.

Comment 19

[Ed Lee :Mardak]

(In reply to comment #18)
> please put the delimiter in a const
Near the top of nsNavHistoryAutoComplete.cpp?

> and document it's purpose
As in comments in this file and/or nsIAutoCompleteSimpleResult.idl?

> also, i'd like to see the test broken out into one specific to this bug.
So make the change to the existing testcase so it passes and do what exactly with the new testcase?

Comment 20

[Ed Lee :Mardak]

Attachment: v2.6

(In reply to comment #18)
> please put the delimiter in a const, and document it's purpose.
Moved it up to a constant and commented.

> also, i'd like to see the test broken out into one specific to this bug.
I'm still not sure what this wants more than what the testcase changes I already have in the patch.

Additionally, per faaborg, I've added commas between the tags.. and additionally I have the tags sorted because right now the tags come back in the order they were added.

Also, to reduce the amount of extra nodes needed for tags plus keywords (bug 392143).. I converted the tag-box into an extra-box that can be reused for tags and keywords.

Comment 21

[Ed Lee :Mardak]

Attachment: screenshot of v2.6

Comment 22

[Ed Lee :Mardak]

If you want to try it out:

https://build.mozilla.org/tryserver-builds/2008-03-15_16:[e-mail]/

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

Comment 23

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

Comment on attachment 309710 [details] [diff] [review]
v2.6

I still don't like the fact that this embeds tag-specific knowledge into a [supposedly-]generic autocomplete binding, and that it overloads the value of getCommentAt, but there probably isn't a simple way around that. Need to think about this a bit more.

Comment 24

[Ed Lee :Mardak]

Attachment: v3

r?dietrich again for making tags always show instead of only if we match them. I just realized that I've been seeing tags because I made tags always show for bug 395161, but that's less likely to land than this which would make the patch more correct.

Also, I added a new testcase that looks for separating title and tags. Additionally updated the test from bug 393678 because it has tag matches.

r?gavin still, but I've moved styling into toolkit style except the rule that undoes the font styling from browser.css.

Comment 25

[Ed Lee :Mardak]

Attachment: v3.1

Unbitrot changes from bug 422491 and bug 422177.

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 311226 [details] [diff] [review]
v3.1

r=me on the places changes. please get ui-r for "always show tags".

Comment 27

[Ed Lee :Mardak]

Comment on attachment 311226 [details] [diff] [review]
v3.1

ui-r? for making tags always show -- instead of only when we match in the tags.

Comment 28

[Ed Lee :Mardak]

Attachment: v3.2

Unbitrot nsNavHistoryAutoComplete.cpp but autocomplete.xml should be the same.

Comment 29

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

Comment on attachment 313790 [details] [diff] [review]
v3.2

Sorry this took so long, I should have pulled the trigger earlier. We can always refactor this code later if the need for a truly generic binding arises (and the code being added here has negligible negative effects for potential rich autocomplete implementations that don't support tags).

Comment 30

[Ed Lee :Mardak]

Comment on attachment 313790 [details] [diff] [review]
v3.2

a1.9? for blocking P2 bug that has plenty of nightly testing with tryserver builds

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 313790 [details] [diff] [review]
v3.2

a=mconnor on behalf of 1.9 drivers

Comment 32

[Ed Lee :Mardak]

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.208; previous revision: 1.207
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.143; previous revision: 1.142
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.198; previous revision: 1.197
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.56; previous revision: 1.55
done
Checking in toolkit/components/places/tests/unit/test_416211.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_416211.js,v  <--  test_416211.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/places/tests/unit/test_416214.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_416214.js,v  <--  test_416214.js
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_418257.js,v
done
Checking in toolkit/components/places/tests/unit/test_418257.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_418257.js,v  <--  test_418257.js
initial revision: 1.1
done
Checking in toolkit/components/places/tests/unit/test_word_boundary_search.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_word_boundary_search.js,v  <--  test_word_boundary_search.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.137; previous revision: 1.136
done
Checking in toolkit/themes/gnomestripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/themes/pinstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.32; previous revision: 1.31
done

Comment 33

[nightstalkerz]

Would it be possible to move the tags onto the bottom line (url line) as the page title is usually longer than the url and having multiple tags obstructs the view of the title.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

I verified this as fixed along with https://bugzilla.mozilla.org/show_bug.cgi?id=401660#c47 at the same time (its litmus test covers this too, essentially).