Closed Bug 485122 Opened 12 years ago Closed 12 years ago

When the user selects to only display history in the location bar, don't display the star and tag icons for items that happen to be bookmarked or tagged

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: faaborg, Assigned: Mardak)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 3 obsolete files)

We need to make a decision on this, right now it feels a little odd to see bookmarks in the location bar after explicitly asking for them not to be displayed.  I think the assumption is probably that the user is combining this preference with actively clearly their history (either on close, or by starting in private browsing mode), so they will only see these items after visiting them in the current session.

One possibility would be to not display a bookmark star for items in the location bar autocomplete results that are only being displayed because they are in history (even if they are in fact bookmarks).
(In reply to comment #0)
> One possibility would be to not display a bookmark star for items in the
> location bar autocomplete results that are only being displayed because they
> are in history (even if they are in fact bookmarks).

I think this is the most natural way to handle this.  Excluding history items because they are bookmarked as well doesn't seem logical to me.
I agree. We obey the user pref and only show history items.. some just happen to be bookmarked.

We can just check the pref and not show a star.
>We can just check the pref and not show a star.

sounds good, adjusting summary
Summary: When the user selects to only display history in the location bar, should we display bookmarks that are in history → When the user selects to only display history in the location bar, don't display stars for items that happen to be bookmarked
Do we want to show tags?
Hardware: x86 → All
(In reply to comment #4)
> Do we want to show tags?

Yes, I think so, because the pref doesn't concern tags.
Right, but the concern is that it's obvious the entry is bookmarked because there are tags next to it.
yeah, and we are trying to establish that when you tag something it is also bookmarked (you can't tag history items), so it probably makes sense to hide the tags as well.
Summary: When the user selects to only display history in the location bar, don't display stars for items that happen to be bookmarked → When the user selects to only display history in the location bar, don't display the star and tag icons for items that happen to be bookmarked or tagged
Attached patch v1 (obsolete) — Splinter Review
I wonder if we should have autocomplete binding call some dummy adjust item method set by urlbarbindings to process the item before being displayed..
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #369460 - Flags: review?(gavin.sharp)
Comment on attachment 369460 [details] [diff] [review]
v1

We shouldn't getService the preferences service for each adjustItem call, and ideally shouldn't get the pref more than once per _invalidate. That pref is also specific to places, and the toolkit autocomplete binding isn't... my patch for bug 480350 adds an "insertion watcher" which could be useful here, I guess.

What's type="favicon"? Don't see that being used anywhere at the moment...
Attachment #369460 - Flags: review?(gavin.sharp) → review-
Attached patch v2 (obsolete) — Splinter Review
> What's type="favicon"? Don't see that being used anywhere at the moment...
It's the default type that nsNavHistoryAutoComplete uses if it isn't anything else.
Attachment #369460 - Attachment is obsolete: true
Attachment #369674 - Flags: review?(gavin.sharp)
Hmm, actually, can't this be fixed entirely from within nsNavHistory::AutoCompleteProcessSearch, by checking for mAutoCompleteCurrentBehavior==kAutoCompleteBehaviorHistory when setting the autocomplete entry's style?
Attached patch v3 (obsolete) — Splinter Review
Switch to NavHistoryAutoComplete implementation with test.
Attachment #369674 - Attachment is obsolete: true
Attachment #371191 - Flags: review?(dietrich)
Attachment #369674 - Flags: review?(gavin.sharp)
That will strip tags if mAutoCompleteCurrentBehavior is kAutoCompleteBehaviorHistory & kAutoCompleteBehaviorTag, won't it? I suggested == for a reason :)
Well, it won't strip /because/ it's "& Tag", but sure, if it's "& History" and "& Tag", it'll strip tags.
Edward points out that I forgot that the bitfield represents a set of restrictions again...

Still, it seems like a query restricted to tagged history entries shouldn't be stripped of the tag indicator.
(In reply to comment #15)
> Still, it seems like a query restricted to tagged history entries shouldn't be
> stripped of the tag indicator.

agreed. my concern is that users will think of these search operators as overrides to the default behavior. eg: "if i restrict to history by default, i'll still be able to search bookmarks and tags via the operators".

if we ignore the operators at any point, then the software is just not doing what the user asked it to. which is bad.
The software is still filtering the results by bookmarks if you type *. It just won't give away that those pages are your bookmarks.

I think it would be more jarring to the user if somebody could come over to his/her machine and hit * and reveal the bookmarks that s/he thought weren't marked as bookmarks.

But again.. this is assuming obscurity is a good way to keep things safe..
We talked about this today at the all-hands. Basically: In Firefox 3 we put a bunch of effort into showing users *why* awesomebar results match, via visual indicators such as the star and tag icons. With this change, when the user explicitly *asks* for bookmarks (for example) we won't show the visual indicator that shows that their search worked.

Here are two possible solutions:

- respect the default behavior, and show the icons only when search operators are used

- override the default behavior when search operators are used
Attached patch v3.1Splinter Review
Pretend a page isn't bookmarked/tagged when searching for only history unless the user explicitly wants them. Test by updating special searches test to ignore tags when searching for history ^.
Attachment #371191 - Attachment is obsolete: true
Attachment #375196 - Flags: review?(dietrich)
Attachment #371191 - Flags: review?(dietrich)
Comment on attachment 375196 [details] [diff] [review]
v3.1

hrm, i'm not happy w/ special-casing this. i think we should move to a model where all special search chars override the default. but that'd require a bigger change than we want to take. this is ok for 3.5, r=me.
Attachment #375196 - Flags: review?(dietrich) → review+
Special vs default isn't the issue here. You can trigger a history only search with ^ which would hide the tags and star.
http://hg.mozilla.org/mozilla-central/rev/033f19d61c13

Pretend a page isn't bookmarked/tagged when searching for only history unless the user explicitly wants them. Test by updating special searches test to ignore tags when searching for history ^.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Comment on attachment 375196 [details] [diff] [review]
v3.1

(In reply to comment #20)
> this is ok for 3.5, r=me.
Attachment #375196 - Flags: approval1.9.1?
Attachment #375196 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 375196 [details] [diff] [review]
v3.1

a191=beltzner, though I think this is potentially confusing in the opposite direction. I'm going to assume you guys have thought the UI design through, here.
verified FIXED on builds:

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

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre ID:20090521030957
Status: RESOLVED → VERIFIED
Depends on: 494224
You need to log in before you can comment on or make changes to this bug.