407601 - Annoyance: always returns to the top of the bookmarks menu

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Nicolas Redfern]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

If the bookmarks menu is closed and then reopened (either by clicking a bookmark, or closing the menu), then it returns to the top of the menu. In previous versions of Firefox, it remained at its previous position. The new behaviour is extremely annoying when using a bookmarks menu that is longer than the screen, because it involves scrolling back through all the bookmarks that you just scrolled through. Please revert to the old behaviour.

Reproducible: Always

Steps to Reproduce:
1. Have lots of bookmarks (more than can be displayed at once)
2. Open the bookmarks menu
3. Scroll down and click on a bookmark
4. Open the bookmarks menu again
Actual Results:  
It was back at the top of the bookmarks menu

Expected Results:  
Remained at the same place in the menu.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120823 Minefield/3.0b2pre

Comment 2

[Nicolas Redfern]

Please could someone else confirm on non-Windows: Linux or OSX

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Confirmed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120804 Minefield/3.0b2pre.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Regression range for this is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1190117820&maxdate=1190126399
With two possible culprits in it: bug 394301 and (less likely) bug 396412.

Comment 5

[Mike Connor [:mconnor]]

I don't think this blocks, but we would gladly take a fix.

Comment 6

[Neil Deakin]

Attachment: possible patch

This is a possible patch and a better fix for bug 394301. It changes the positioning to use the original preferred size when calculating the position and size rather than the current size, as the current size can change if the popup is too close to the edge of the screen. This led to a size change and an underflow event being fired which caused the scrollbox to scroll to the top.

Comment 9

[Neil Deakin]

Attachment: same as last patch, but modify a test so it checks the scroll position

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm not sure when I'll be able to get to this... might well not be until January if not later.  Part of the problem is that this code is complex and I just don't remember it very well, so would have to relearn it.

Some explanation of why the various parts are needed (if there is more to it than what is described in comment 6) would go a long way toward making this easier to review.

Comment 11

[Neil Deakin]

Although the patch looks big, it actually just changes the references to the size fields of mRect to a use different rect mPrefSize, so that positon and size computations are performed on the preferred size (returned by nsBox::GetPrefSize) rather than the current size in mRect. This is because the preferred size only changes when the content changes size, whereas mRect can change size (within SetPopupPosition) due to various things, like not enough screen space, the scroll arrows appearing on the menu, etc.

This is because we want to computations to be based on the original preferred size, rather than the current size which keeps changing.

This is only a wanted bug though.

Comment 12

[Marco Bonardo [:mak]]

Neil, could this fix the problem with drag&drop in bookmarks menu where the drop point is calculated without counting the popup movement due to not enough screen space? if so this is a wanted bug that can solve a blocker (i'm talking about Bug 405087), when comparing y on repositioned submenus they have still old y values.

Comment 13

[Neil Deakin]

Comment on attachment 293536 [details] [diff] [review]
same as last patch, but modify a test so it checks the scroll position

Found an issue with this patch anyway, and have a much simpler patch

Comment 14

[Neil Deakin]

Attachment: this patch is simpler and just initializes mRect to the preferred size before positioning/sizing the popup

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293676 [details] [diff] [review]
this patch is simpler and just initializes mRect to the preferred size before positioning/sizing the popup

r+sr=bzbarsky

Comment 16

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122005 Minefield/3.0b3pre

Comment 20

[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