Closed
Bug 418257
Opened 17 years ago
Closed 17 years ago
Show what part of which tags match for urlbar autocomplete
Categories
(Firefox :: Address Bar, defect, P2)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 3
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
Attachments
(4 files, 14 obsolete files)
(deleted),
image/png
|
beltzner
:
ui-review+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gavin
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
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•17 years ago
|
Assignee: nobody → edilee
Whiteboard: [has fix in bug 401660]
Assignee | ||
Comment 1•17 years ago
|
||
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•17 years ago
|
||
Initial stab at tags with tag icon and bookmark star. No styling yet.
Assignee | ||
Comment 3•17 years ago
|
||
Assignee | ||
Comment 4•17 years ago
|
||
Assignee | ||
Comment 5•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Whiteboard: [has fix in bug 401660] → [has patch][needs review gavin]
Comment 7•17 years ago
|
||
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•17 years ago
|
||
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 | ||
Comment 9•17 years ago
|
||
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•17 years ago
|
||
Icons aren't super tall anymore. Before they were 16x19 without the align center.
Updated•17 years ago
|
Attachment #307638 -
Attachment is patch: false
Attachment #307638 -
Attachment mime type: text/plain → image/png
Assignee | ||
Comment 11•17 years ago
|
||
Unbitrot from bug 421412.
Attachment #307637 -
Attachment is obsolete: true
Attachment #307637 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 12•17 years ago
|
||
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•17 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?
Comment 14•17 years ago
|
||
Now that we're matching partial tags, this is pretty important for discoverability.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Assignee | ||
Comment 15•17 years ago
|
||
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)
Comment 16•17 years ago
|
||
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 18•17 years ago
|
||
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•17 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•17 years ago
|
||
(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•17 years ago
|
||
Assignee | ||
Comment 22•17 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•17 years ago
|
Whiteboard: [has patch][needs review gavin] → [has patch][need review gavin][need bug 407946]
Comment 23•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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 26•17 years ago
|
||
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•17 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)
Updated•17 years ago
|
Attachment #311226 -
Flags: ui-review?(beltzner) → ui-review+
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][need review gavin][need bug 407946] → [has patch][need review gavin][needed for bug 424216]
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Updated•17 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•17 years ago
|
||
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 29•17 years ago
|
||
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+
Updated•17 years ago
|
Whiteboard: [has patch][need review gavin] → [has patch]
Assignee | ||
Comment 30•17 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 31•17 years ago
|
||
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•17 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
Closed: 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch]
Comment 33•17 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.
Updated•17 years ago
|
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.
Description
•