The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.8.1

Status

()

Toolkit
XUL Widgets
--
minor
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Simon Bünzli, Assigned: Simon Bünzli)

Tracking

({fixed1.8.1, regression})

1.8 Branch
mozilla1.8.1
fixed1.8.1, regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Component: Extension/Theme Manager → XUL Widgets
Product: Firefox → Toolkit
QA Contact: extension.manager → xul.widgets
(Assignee)

Comment 1

12 years ago
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.
Keywords: regression
Summary: When moving an extension up/down, it is scrolled to the top of the window. → When moving an extension up/down in the richlistbox widget, it is scrolled to the top of the window.
(Assignee)

Comment 2

12 years ago
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.
Attachment #196777 - Flags: first-review?(mconnor)

Comment 3

12 years ago
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.
Attachment #196777 - Flags: second-review+
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...
(Assignee)

Comment 5

12 years ago
Created attachment 196783 [details] [diff] [review]
nit picked

Sorry for that.

Updated

12 years ago
Attachment #196783 - Flags: first-review+

Updated

12 years ago
Attachment #196777 - Flags: first-review?(mconnor)
Does this need to be checked in? On the branch, too?
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?
(Assignee)

Comment 8

11 years ago
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.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
mozilla/toolkit/content/widgets/richlistbox.xml 	1.13.2.10
mozilla/toolkit/content/widgets/richlistbox.xml 	1.28
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1
Version: unspecified → 1.8 Branch
Attachment #196777 - Attachment is obsolete: true
Attachment #196783 - Attachment is obsolete: true
Attachment #214897 - Attachment description: patch for trunk and branch → patch for trunk and branch (checked in)
(Assignee)

Comment 10

11 years ago
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.