Bookmarks menu opens wrong bookmark

VERIFIED FIXED in mozilla1.9alpha8

Status

()

VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Dolske, Assigned: jaas)

Tracking

({regression})

Trunk
mozilla1.9alpha8
x86
Mac OS X
regression
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 2

11 years ago
Does this also happen on Linux or Windows?
> 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.
Keywords: regression
(Reporter)

Comment 4

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

Comment 5

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

Updated

11 years ago
Duplicate of this bug: 394523
(Reporter)

Updated

11 years ago
Duplicate of this bug: 394399

Comment 8

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

Comment 10

11 years ago
Yes, when adding the star I'm also clicking the star again so I can change the folder to "Bookmarks Menu".
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)
> 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.)
Flags: blocking-firefox3?
(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.

> 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.
Assignee: nobody → cbarrett
Target Milestone: --- → Firefox 3 M8
(Assignee)

Comment 15

11 years ago
Created attachment 279272 [details] [diff] [review]
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.
Assignee: cbarrett → joshmoz
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Status: ASSIGNED → NEW
Component: Places → Widget: Cocoa
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: places → cocoa
Target Milestone: Firefox 3 M8 → mozilla1.9 M8
(Assignee)

Updated

11 years ago
Flags: blocking1.9+
(Assignee)

Comment 16

11 years ago
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.
Attachment #279272 - Flags: review?(cbarrett)
Comment on attachment 279272 [details] [diff] [review]
fix v1.0

Ahh, d'oh. Stupid separators.
Attachment #279272 - Flags: review?(cbarrett) → review+
(Assignee)

Updated

11 years ago
Attachment #279272 - Flags: superreview?(pavlov)
Comment on attachment 279272 [details] [diff] [review]
fix v1.0

I need this fix!
Attachment #279272 - Flags: superreview?(pavlov) → superreview+
(Assignee)

Comment 19

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.