394515 - Bookmarks menu opens wrong bookmark

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Justin Dolske [:Dolske]]

I just noticed this, so it probably regressed in the last day or two.

If I select an entry from the Bookmarks menu, the page I get is always from the bookmark 2 entries down. This does not happen with folder menus on the Bookmarks Toolbar. Nor does it happen when selecting entries from subfolders in the Bookmarks menu.

Eg:
Bookmarks
   Foo
   Bar
   Baz

If I select Foo, the page that loads is actually Baz. Selecting either of the last two entries in the menu also gives me Baz.

Comment 1

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

fwiw, I don't see this on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007083105 Minefield/3.0a8pre

dolske, do you see it with a fresh profile?

Comment 2

[Josh Aas]

Does this also happen on Linux or Windows?

Comment 3

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

> Does this also happen on Linux or Windows?

I don't see this on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a8pre) Gecko/2007083105 Minefield/3.0a8pre

> dolske, do you see it with a fresh profile?

<dolske> sspitzer: yeah, happens on a new profile too.

Comment 4

[Justin Dolske [:Dolske]]

I can reproduce this on a new profile.
1) Create new profile
2) Bookmark 3 sites (via Star icon, if that matters)
3) Select first bookmark from Bookmarks menu
4) Page actually loads 3rd bookmark.

Comment 5

[Justin Dolske [:Dolske]]

Maybe due to https://bugzilla.mozilla.org/attachment.cgi?id=277498&action=diff ?

Mac only, patch is dealing with menu code, adds methods like:

    NS_IMETHOD GetVisibleItemCount(PRUint32 &aCount);
    NS_IMETHOD GetVisibleItemAt(const PRUint32 aPos, nsISupports *& aMenuItem);

Comment 8

[Adam Vandenberg]

I just deleted one of the two separators in my Bookmarks menu, and now I'm
getting bookmarks one item down instead of two, so I'd say that "visible item
count" change is a good bet.

Comment 9

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

justin, for your steps in comment #4, are the three sites you are bookmarking showing up as menu items of the bookmarks menu?

I'm not seeing them (bug #393944)

Comment 10

[Justin Dolske [:Dolske]]

Yes, when adding the star I'm also clicking the star again so I can change the folder to "Bookmarks Menu".

Comment 11

[Colin Barrett [:cbarrett]]

I can only reproduce this *some* of the time. Sometimes it's only off by one, sometimes it's off by none.

I will take a look at it this weekend. I have some ideas about what could be wrong here. (I really wish we had unit tests for this stuff :\)

(btw the click UI for the star SUCKS. it's some sort of weird tri-state button now, ick)

Comment 12

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

> I can only reproduce this *some* of the time. Sometimes it's only off by one,
> sometimes it's off by none.

colin, have you determined which patch caused the regression?  was it bug #382194, as dolske thought?  (If so, perhaps we should back it out.)

> btw the click UI for the star SUCKS. it's some sort of weird tri-state button
> now, ick

instead of using this bug for your feedback, I'd suggestusing dev.app.firefox (faaborg is super responsive) or to https://bugzilla.mozilla.org/show_bug.cgi?id=393509 (which I think tracks the most recent mockup for what he is thinking.)

Comment 13

[Colin Barrett [:cbarrett]]

(In reply to comment #12)
> > I can only reproduce this *some* of the time. Sometimes it's only off by one,
> > sometimes it's off by none.
> 
> colin, have you determined which patch caused the regression?  was it bug
> #382194, as dolske thought?  (If so, perhaps we should back it out.)

No, I don't know for sure. I was going to try backing out bug 382194 locally and seeing if I could still reproduce.
 
> > btw the click UI for the star SUCKS. it's some sort of weird tri-state button
> > now, ick
> 
> instead of using this bug for your feedback, I'd suggestusing dev.app.firefox
> (faaborg is super responsive) or to
> https://bugzilla.mozilla.org/show_bug.cgi?id=393509 (which I think tracks the
> most recent mockup for what he is thinking.)

It was out of place, I apologize. I'll direct my feedback (phrased more constructively) to the appropriate place. Thanks for your patience.

Comment 14

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

> No, I don't know for sure. I was going to try backing out bug 382194 locally
> and seeing if I could still reproduce.

I haven't gone as far as to back you out locally, but I did find a regression range.

I don't see this bug with: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007-08-27-04-trunk/firefox-3.0a8pre.en-US.mac.dmg

I do see this bug with:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007-08-28-04-trunk/firefox-3.0a8pre.en-US.mac.dmg

Your check in for bug #382194 was on 2007-08-27 17:57.

There were places checkins during that time (but none of them look suspicious).  Note, there were other checkins, see:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-27+00%3A00%3A00&maxdate=2007-08-28+05%3A00%3A00&cvsroot=%2Fcvsroot

(If it wasn't your menu change, maybe bug #392160?)

colin, I'm going to re-assign to you for now.  since this is a m8 regression, targeting as an m8 blocker.

Comment 15

[Josh Aas]

Attachment: fix v1.0

This fixes the problem. I'll request review after I've pondered deeper issues with our separator handling for the rest of the weekend.

Comment 16

[Josh Aas]

Comment on attachment 279272 [details] [diff] [review]
fix v1.0

The deeper issues I was talking about are bug 375011. We should probably fix that soon but it doesn't have anything to do with this fix.

Comment 17

[Colin Barrett [:cbarrett]]

Comment on attachment 279272 [details] [diff] [review]
fix v1.0

Ahh, d'oh. Stupid separators.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 279272 [details] [diff] [review]
fix v1.0

I need this fix!

Comment 19

[Josh Aas]

landed on trunk

Comment 20

[Carsten Book [:Tomcat]]

Verified fixed using the steps to reproduce from justin (comment #4) and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007123104 Minefield/3.0b3pre

- Verified fixed