387718 - the location bar dropdown should show a star next to bookmarked results

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

if a result in the location bar dropdown is bookmarks, it should show a star in the far right end of the result.

Comment 1

[(not reading, please use [e-mail] instead)]

taking

Comment 2

[bomfog]

Getting together with bug 389252 (add visual indicator if current URL is already bookmarked) might save some duplicated effort, maybe?

Comment 3

[bomfog]

Sorry, shoulda looked closer, didn't notice the Blocks:.

Comment 4

[(not reading, please use [e-mail] instead)]

accepting.  depends on by some work in bug #389491.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: screen shot (need to fix the flex, but getting close)

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: updated screen shot

Comment 7

[Bryan]

(In reply to comment #6)
> Created an attachment (id=277820) [details]
> updated screen shot

Seth, can we nudge the star to the left a pixel or two and maybe up one so it's not touching the far edge of the address bar?

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: patch

patch, v1.

Comment 9

[(not reading, please use [e-mail] instead)]

I'm not super happy with how I've had to set the style of the image column to be "max-width: 36px; padding-end: 18px;"

the three columns have flex 2, 1, 1 respectively (url, comment, image), and the star icons is 16 x 16 px.

but, the scrollbar cuts off the end of the last column

note, now that we have a third column, you will see elipisis on the comment column (which could have been hidden by the scroll bar).  see "slashdot" in the screen shot attached to this bug.

gavin / mano / others:  any suggestions on how to deal with this in a cleaner way?

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: updated patch (move column style to browser.css)

(need to test on mac)

Comment 11

[(not reading, please use [e-mail] instead)]

> (need to test on mac)

tested that patch on the mac, please see the second screen shot.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: screen shot from the mac

Comment 13

[(not reading, please use [e-mail] instead)]

note, this patch suffers from bug #393887, but fixing that will be a change to places and independent of this patch, so I'm working on that in parallel.

Comment 14

[(not reading, please use [e-mail] instead)]

> this patch suffers from bug #393887

s/patch/feature

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: address bug #393887 for autocomplete results

note, you can't see this in action without the patch (pending review), but the effect is that rss items

Comment 16

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

Comment on attachment 277859 [details] [diff] [review]
updated patch (move column style to browser.css)

>Index: toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl

>+  /*
>+   * Option to show a third column in the popup which contains
>+   * an additional image for each autocomplete result
>+   */
>+  attribute unsigned long showImageColumn;

Why is this "unsigned long" and not boolean, anyways? I suppose you're just copying showCommentColumn, but if there's no good reason for it to be that way we should just use boolean. (Note also that your autocomplete.xml implementation of this attribute treats it as a boolean, not an integer.)

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, bump the uuid, the attribute type issue (per gavin)

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: updated screen shot, note star button in url bar has moved locations

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 279030 [details] [diff] [review]
address bug #393887 for autocomplete results

going to move this to a separate bug and seek review there.

Comment 20

[(not reading, please use [e-mail] instead)]

fixed.

Checking in toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl
,v  <--  nsIAutoCompleteInput.idl
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/satchel/src/nsFormFillController.cpp;
/cvsroot/mozilla/toolkit/components/satchel/src/nsFormFillController.cpp,v  <--
 nsFormFillController.cpp
new revision: 1.89; previous revision: 1.88
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.82; previous revision: 1.81
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.365; previous revision: 1.364
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.85; previous revision: 1.84
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.70; previous revision: 1.69
done

Comment 21

[(not reading, please use [e-mail] instead)]

> going to move this to a separate bug and seek review there.

spun off to bug #394484

Comment 22

[Peter Van der Beken [:peterv]]

This seems to have caused the following error:

CSS Error (chrome://browser/skin/browser.css :982.14): Unknown property 'padding-end'.  Declaration dropped.

I guess you meant -moz-padding-end?

Comment 23

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

(In reply to comment #22)
> This seems to have caused the following error:

I just removed the broken style rule, given that it's apparently unnecessary.

Comment 24

[(not reading, please use [e-mail] instead)]

peter / gavin:  thanks for catching and fixing that broken style rule.

Comment 25

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre.  Star appears in location bar dropdown next to bookmarked sites. 

Comment 26

[Marcia Knous [:marcia]]

Litmus Triage Team: adding al and tracy to the bug for Litmus coverage.

Comment 27

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

http://litmus.mozilla.org/show_test.cgi?id=4678

in-litmus+ (sorry; not trying to steal anyone's thunder, but I don't think you'll mind.  If the testcase needs correction, please fix and/or let me know.)

Comment 28

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv