Last Comment Bug 309289 - When moving an extension up/down in the richlistbox widget, it is scrolled to the top of the window.
: When moving an extension up/down in the richlistbox widget, it is scrolled to...
Status: VERIFIED FIXED
: fixed1.8.1, regression
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: 1.8 Branch
: All All
-- minor (vote)
: mozilla1.8.1
Assigned To: Simon Bünzli
:
: Neil Deakin
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-20 04:27 PDT by Simon Bünzli
Modified: 2007-01-08 10:55 PST (History)
6 users (show)
benjamin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Check for visibility before scrolling (1.79 KB, patch)
2005-09-20 05:20 PDT, Simon Bünzli
doronr: second‑review+
Details | Diff | Splinter Review
nit picked (1.80 KB, patch)
2005-09-20 06:29 PDT, Simon Bünzli
mconnor: first‑review+
Details | Diff | Splinter Review
patch for trunk and branch (checked in) (1.18 KB, patch)
2006-03-13 05:34 PST, Simon Bünzli
no flags Details | Diff | Splinter Review

Description User image Simon Bünzli 2005-09-20 04:27:22 PDT
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 User image Simon Bünzli 2005-09-20 04:36:47 PDT
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 User image Simon Bünzli 2005-09-20 05:20:00 PDT
Created attachment 196777 [details] [diff] [review]
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 User image Doron Rosenberg (IBM) 2005-09-20 05:54:51 PDT
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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2005-09-20 06:19:44 PDT
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 User image Simon Bünzli 2005-09-20 06:29:36 PDT
Created attachment 196783 [details] [diff] [review]
nit picked

Sorry for that.
Comment 6 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-03-13 04:56:47 PST
Does this need to be checked in? On the branch, too?
Comment 7 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-03-13 05:00:31 PST
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 User image Simon Bünzli 2006-03-13 05:34:09 PST
Created attachment 214897 [details] [diff] [review]
patch for trunk and branch (checked in)

Yes; only this part has to be checked in to trunk and branch.
Comment 9 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2006-03-13 05:38:34 PST
mozilla/toolkit/content/widgets/richlistbox.xml 	1.13.2.10
mozilla/toolkit/content/widgets/richlistbox.xml 	1.28
Comment 10 User image Simon Bünzli 2006-08-29 09:12:54 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.