309289 - When moving an extension up/down in the richlistbox widget, it is scrolled to the top of the window.

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Simon Bünzli]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050919 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050919 Firefox/1.4

Tested with the extensions manager only, but I suppose it applies to the themes
manager as well.

Reproducible: Always

Steps to Reproduce:
1. Make sure that you've got at least two view port heights of extensions
installed (at least 4, if you display only two at the time).
2. Select the top-most extension, right click it and select Move Down.
3. Select the first extension not visible on screen, right click it and select
Move Up.

Actual Results:  
The extensions are correctly moved, but always scrolled to the top of the view port.

Expected Results:  
Scrolling should only happen, if the extension won't be visible anymore.

Comment 1

[Simon Bünzli]

I'd guess that the problem lies in the fact that the neither the currently
selected index nor the current scroll position are saved before the template
rebuilding takes place in richlistbox.xml. Probably enhancing _builderListener
by this (saving on willRebuild, restoring on didRebuild) might take care of
this. If it does, a patch will be ready soon.

Comment 2

[Simon Bünzli]

Attachment: Check for visibility before scrolling

Seems that the scrolling position is retained already - so it comes down to
checking whether the (newly) selected element is already visible.

BTW: it seems that I've found another bug in my code from Bug 305730. I've
fixed it here, but will make sure that should only Bug 305730 make it into the
branch, it'll be fixed there, too.

Comment 3

[Doron Rosenberg (IBM)]

Comment on attachment 196777 [details] [diff] [review]
Check for visibility before scrolling

r=me, I thought I had that in my patch, must have lost that part.

Comment 4

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

Comment on attachment 196777 [details] [diff] [review]
Check for visibility before scrolling

>Index: richlistbox.xml

>               if (element) {
>                 this.selectedItem = element;
>-                this.scrollBoxObject.scrollToElement(this.selectedItem);
>+                if (!this._isItemVisible(element))
>+                  this.scrollBoxObject.scrollToElement(this.selectedItem);
>                 return;
>               }

It'd be clearer to use either "element" or "this.selectedItem" consistently,
no? Unless I'm missing something...

Comment 5

[Simon Bünzli]

Attachment: nit picked

Sorry for that.

Comment 6

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

Does this need to be checked in? On the branch, too?

Comment 7

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

I guess the second hunk of this patch landed on the branch in bug 305730, so the first hunk of this patch needs landing on the trunk and branch, while the second hunk will be landed on the trunk in bug 323818. Simon, is my understanding correct?

Comment 8

[Simon Bünzli]

Attachment: patch for trunk and branch (checked in)

Yes; only this part has to be checked in to trunk and branch.

Comment 9

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

mozilla/toolkit/content/widgets/richlistbox.xml 	1.13.2.10
mozilla/toolkit/content/widgets/richlistbox.xml 	1.28

Comment 10

[Simon Bünzli]

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060317 Firefox/2.0a1 ID:2006031705. Note that in later builds this issue can't be reproduced easily due to the replacement of the Extensions Manager with the new Add-ons manager.