Show what part of which tags match for urlbar autocomplete

VERIFIED FIXED in Firefox 3

Status

()

P2
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 14 obsolete attachments)

33.83 KB, image/png
beltzner
: ui-review+
Details
70.30 KB, image/png
Details
67.49 KB, image/png
Details
23.87 KB, patch
Gavin
: review+
mconnor
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Posted image screenshot of bug 401660 v1 (obsolete) —
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.
(Assignee)

Updated

11 years ago
Assignee: nobody → edilee
Whiteboard: [has fix in bug 401660]
(Assignee)

Comment 1

11 years ago
Posted image screenshot of v1 (obsolete) —
Initial tweak without any styling: Forced ()s and tag inside "(".

Perhaps margin-left of the tag hbox and padding around the tag image?
Attachment #304049 - Attachment is obsolete: true
(Assignee)

Comment 2

11 years ago
Posted patch v1 (obsolete) — Splinter Review
Initial stab at tags with tag icon and bookmark star. No styling yet.
(Assignee)

Comment 3

11 years ago
Posted image screenshot of v1.1 (obsolete) —
(Assignee)

Comment 4

11 years ago
Posted patch v1.1 (obsolete) — Splinter Review
(Assignee)

Comment 5

11 years ago
Posted image screenshot of v2
Now with more spacing and correct ellipsis handling. :)
Attachment #305831 - Attachment is obsolete: true
Attachment #305843 - Attachment is obsolete: true
Attachment #305864 - Flags: ui-review?(beltzner)
(Assignee)

Comment 6

11 years ago
Posted patch v2 (obsolete) — Splinter Review
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).
Attachment #305833 - Attachment is obsolete: true
Attachment #305845 - Attachment is obsolete: true
Attachment #305866 - Flags: review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Whiteboard: [has fix in bug 401660] → [has patch][needs review gavin]
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.
Attachment #305864 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 8

11 years ago
Posted patch v2.1 (obsolete) — Splinter Review
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 "...").
Attachment #305866 - Attachment is obsolete: true
Attachment #306398 - Flags: review?(gavin.sharp)
Attachment #305866 - Flags: review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Blocks: 392143
(Assignee)

Comment 9

11 years ago
Posted patch v2.2 (obsolete) — Splinter Review
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.
Attachment #306398 - Attachment is obsolete: true
Attachment #307637 - Flags: review?(gavin.sharp)
Attachment #306398 - Flags: review?(gavin.sharp)
(Assignee)

Comment 10

11 years ago
Posted image screenshot of v2.2
Icons aren't super tall anymore. Before they were 16x19 without the align center.

Updated

11 years ago
Attachment #307638 - Attachment is patch: false
Attachment #307638 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 11

11 years ago
Posted patch v2.3 (obsolete) — Splinter Review
Unbitrot from bug 421412.
Attachment #307637 - Attachment is obsolete: true
Attachment #307637 - Flags: review?(gavin.sharp)
(Assignee)

Comment 12

11 years ago
Posted patch v2.4 (obsolete) — Splinter Review
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.
Attachment #307970 - Attachment is obsolete: true
Attachment #308273 - Flags: review?(gavin.sharp)
(Assignee)

Comment 13

11 years ago
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.
Flags: blocking-firefox3?
Now that we're matching partial tags, this is pretty important for discoverability.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
(Assignee)

Comment 15

11 years ago
Posted patch v2.5 (obsolete) — Splinter Review
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.
Attachment #308273 - Attachment is obsolete: true
Attachment #308655 - Flags: review?(gavin.sharp)
Attachment #308655 - Flags: review?(dietrich)
Attachment #308273 - Flags: review?(gavin.sharp)
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.
(Assignee)

Comment 17

11 years ago
Oh bah bah bah bah. This needs bug 407946.
Depends on: 407946
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.
Attachment #308655 - Flags: review?(dietrich) → review+
(Assignee)

Comment 19

11 years ago
(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?
(Assignee)

Comment 20

11 years ago
Posted patch v2.6 (obsolete) — Splinter Review
(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.
Attachment #308655 - Attachment is obsolete: true
Attachment #309710 - Flags: review?(gavin.sharp)
Attachment #308655 - Flags: review?(gavin.sharp)
(Assignee)

Comment 21

11 years ago
Posted image screenshot of v2.6
(Assignee)

Comment 22

11 years ago
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
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review gavin] → [has patch][need review gavin][need bug 407946]
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.
(Assignee)

Comment 24

11 years ago
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #309710 - Attachment is obsolete: true
Attachment #310940 - Flags: review?(gavin.sharp)
Attachment #310940 - Flags: review?(dietrich)
Attachment #309710 - Flags: review?(gavin.sharp)
(Assignee)

Comment 25

11 years ago
Posted patch v3.1 (obsolete) — Splinter Review
Unbitrot changes from bug 422491 and bug 422177.
Attachment #310940 - Attachment is obsolete: true
Attachment #311226 - Flags: review?(gavin.sharp)
Attachment #311226 - Flags: review?(dietrich)
Attachment #310940 - Flags: review?(gavin.sharp)
Attachment #310940 - Flags: review?(dietrich)
Comment on attachment 311226 [details] [diff] [review]
v3.1

r=me on the places changes. please get ui-r for "always show tags".
Attachment #311226 - Flags: review?(dietrich) → review+
(Assignee)

Comment 27

11 years ago
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.
Attachment #311226 - Flags: ui-review?(beltzner)
Attachment #311226 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Updated

11 years ago
Blocks: 424216
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][need review gavin][need bug 407946] → [has patch][need review gavin][needed for bug 424216]
(Assignee)

Updated

11 years ago
No longer blocks: 424216
Depends on: 424216
(Assignee)

Updated

11 years ago
Flags: in-testsuite?
Whiteboard: [has patch][need review gavin][needed for bug 424216] → [has patch][need review gavin]
Target Milestone: --- → Firefox 3
(Assignee)

Comment 28

11 years ago
Posted patch v3.2Splinter Review
Unbitrot nsNavHistoryAutoComplete.cpp but autocomplete.xml should be the same.
Attachment #311226 - Attachment is obsolete: true
Attachment #313790 - Flags: review?(gavin.sharp)
Attachment #311226 - Flags: review?(gavin.sharp)
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).
Attachment #313790 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need review gavin] → [has patch]
(Assignee)

Comment 30

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

a1.9? for blocking P2 bug that has plenty of nightly testing with tryserver builds
Attachment #313790 - Flags: approval1.9?
Comment on attachment 313790 [details] [diff] [review]
v3.2

a=mconnor on behalf of 1.9 drivers
Attachment #313790 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 32

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch]

Comment 33

11 years ago
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.
Flags: in-litmus?
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).
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.