Closed
Bug 306522
Opened 20 years ago
Closed 20 years ago
richlistbox needs to use template listeners
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.8beta4
People
(Reporter: doronr, Assigned: doronr)
References
Details
(Keywords: fixed1.8)
Attachments
(3 files, 4 obsolete files)
|
21.01 KB,
patch
|
asaf
:
first-review+
|
Details | Diff | Splinter Review |
|
21.90 KB,
patch
|
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
|
16.43 KB,
patch
|
Details | Diff | Splinter Review |
See my backed out patch in https://bugzilla.mozilla.org/show_bug.cgi?id=305667
|
||
Comment 1•20 years ago
|
||
If we want to ship at least a semi-stable widget, I believe it should be done by
b4, see bug 306299.
Flags: blocking1.8b4?
|
Assignee | |
Comment 2•20 years ago
|
||
This is 1.8 only - won't apply on trunk due to aaronl's other patch.
This seems to fix all the issues I've seen reported.
Attachment #194437 -
Flags: first-review?(bugs.mano)
|
||
Comment 3•20 years ago
|
||
Asaf, when requesting approval to be a beta stopper, it would be helpful if you
could right a summary about why you are nominating it, why we need the fix. That
helps the bug traige team out a lot when we go through a bunch of bugs. Thanks!
|
|
Assignee | |
Comment 4•20 years ago
|
||
Attachment #194437 -
Attachment is obsolete: true
Attachment #194441 -
Flags: first-review?(bugs.mano)
|
|
||
Comment 5•20 years ago
|
||
Comment on attachment 194441 [details] [diff] [review]
with irc comments addressed (still 1.8 only)
Haven't tested this yet; drive-by comments:
> <constructor>
> <![CDATA[
> var x = document.getAnonymousElementByAttribute(this, "anonid", "main-box");
> this.scrollBoxObject = x.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+
>+ // add a template build listener
>+ this.builder.addListener(this._builder());
> ]]>
> </constructor>
What about plain-richlistbox with persist="last-selected" set?
>+ <method name="_builder">
>+ <body>
>+ <![CDATA[
>+ var myThis = this;
>+ return {
>+ item: null,
>+ willRebuild : function(builder) {},
>+ didRebuild : function(builder) {
>+ myThis._refreshSelection()
>+ }
>+ }
>+ ]]>
>+ </body>
>+ </method>
This should be implemented as a <filed>; also, a better name would be
_builderListener.
>+
>+ <method name="_refreshSelection">
>+ <body>
>+ <![CDATA[
>+ // fist look for a last-selected attribute
>+ var lastSelected = this.getAttribute("last-selected");
>+ if (lastSelected != "") {
>+ var element = document.getElementById(lastSelected);
>+
>+ if (element)
>+ this.selectedItem = element;
>+
>+ if (this.selectedItem)
>+ this.scrollBoxObject.scrollToElement(this.selectedItem);
>+
>+ return;
>+ }
>+
how about:
if (lastSelected != "") {
var element = document.getElementById(lastSelected);
if (element) {
this.selectedItem = element;
this.scrollBoxObject.scrollToElement(this.selectedItem);
return;
}
}
I don't think you want to return if you haven't changed selectedItem.
>+ if (this.selectedItem) {
>+ if (this.selectedItem.hasAttribute("id")) {
>+ var id = this.selectedItem.getAttribute("id");
>+ var item = document.getElementById(id);
>+
>+ // if we find no item, clear selection so that the code at the bottom
>+ // takes over
>+ if (item)
>+ this.selectedItem = item;
>+ else
>+ this.clearSelection();
>+ } else {
>+ // if no id, we clear selection so that the below code will select
>+ // based on the current index
>+ this.clearSelection();
>+ }
>+ }
ah? aren't you doing too much work here? if the selected item was removed, you
should be able to test if this.selectedItem belongs to any document... instead
of getting it from the current document via its id.
>Index: toolkit/mozapps/downloads/content/downloads.js
>===================================================================
>@@ -416,18 +426,22 @@
...
>+
> gDownloadViewController.onCommandUpdate();
>+
>+ // retry always places the item in the first spot
>+ gDownloadsView.selectedIndex = 0;
>+focus();
focus()?
>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================
>
> // Initialize the richlistbox. This will select the last selected extension
> // or default to the first listed one.
>- gExtensionsView.init();
>+ //gExtensionsView.init();
remove the commented line.
Attachment #194441 -
Flags: first-review?(bugs.mano) → first-review-
|
|
||
Updated•20 years ago
|
Attachment #194437 -
Flags: first-review?(bugs.mano)
|
|
||
Comment 6•20 years ago
|
||
(In reply to comment #3)
> Asaf, when requesting approval to be a beta stopper, it would be helpful if you
> could right a summary about why you are nominating it, why we need the fix. That
> helps the bug traige team out a lot when we go through a bunch of bugs. Thanks!
FWIW, raeasoning here was/is the new borken richlistbox API which leads to bugs
like 306299.
|
|
||
Comment 7•20 years ago
|
||
Comment on attachment 194441 [details] [diff] [review]
with irc comments addressed (still 1.8 only)
Also, can you move the last-selected implementation bits from extensions.js to
richlistbox?
|
|
Assignee | |
Comment 8•20 years ago
|
||
> ah? aren't you doing too much work here? if the selected item was removed, you
> should be able to test if this.selectedItem belongs to any document... instead
> of getting it from the current document via its id.
>
_refreshSelection is always called when we know we need to reset selection.
Either on creation of the widget or during template rebuilds.
|
|
Assignee | |
Comment 9•20 years ago
|
||
Attachment #194441 -
Attachment is obsolete: true
Attachment #194538 -
Flags: first-review?(bugs.mano)
|
|
||
Comment 10•20 years ago
|
||
Comment on attachment 194538 [details] [diff] [review]
with comments addressed
+ <field name="_builderListener">
>+ <
|
||
Comment 12•20 years ago
|
||
we want this fix before we ship 1.5 so we can have a good API here, and the
sooner the better, but pushing it out to a beta 2 blocker since it's not yet
ready to land.
Flags: blocking1.8b5+
Flags: blocking1.8b4?
Flags: blocking1.8b4-
|
|
Assignee | |
Comment 13•20 years ago
|
||
I tested this pretty decently and all seems well.
Attachment #194538 -
Attachment is obsolete: true
Attachment #194578 -
Flags: first-review?(bugs.mano)
|
|
Assignee | |
Updated•20 years ago
|
Attachment #194578 -
Attachment is obsolete: true
Attachment #194578 -
Flags: first-review?(bugs.mano)
|
|
Assignee | |
Comment 14•20 years ago
|
||
Attachment #194597 -
Flags: first-review?(bugs.mano)
|
|
||
Comment 15•20 years ago
|
||
Comment on attachment 194597 [details] [diff] [review]
puts back the last-selected setting code into richlistbox, 1.8 only still
Index: toolkit/content/widgets/richlistbox.xml
===================================================================
...
+ <method name="_refreshSelection">
...
+ if (item) {
+ this.selectedItem = item;
+ this.ensureSelectedElementIsVisible();
Remove this call (it was already done).
+ } else {
fix indent.
...
+ if (selectedIndex >= this.getRowCount())
+ this.selectedIndex = this.getRowCount() - 1;
+ else
+ this.selectedIndex = selectedIndex;
+
+ this.ensureSelectedElementIsVisible();
see above.
...
if (val) {
- val.selected = true;
- this.scrollBoxObject.ensureElementIsVisible(val);
this.fireActiveItemEvent();
}
remove single line brackets.
...
<destructor>
<![CDATA[
// when we are destructed and we are selected, unselect ourselves so
// that richlistbox's selection doesn't point to something not in
the DOM.
if (this.selected) {
- this.control.clearSelection();
+ this.control._setItemSelection(null);
Maybe add a comment here explaing we don't want last-selected to be modified
(i.e. why we're not using clearSelection).
Index: toolkit/mozapps/downloads/content/downloads.js
===================================================================
...
+ gDownloadsView.selectedIndex = selectedIndex
missing semicolon.
r=mano with those fixed (please do attach le final patch).
Attachment #194597 -
Flags: first-review?(bugs.mano) → first-review+
|
|
Assignee | |
Comment 16•20 years ago
|
||
I had to keep the final ensureSelectedElementIsVisible() because it works
around a bug in scrollbox, I added an XXX: comment.
|
|
Assignee | |
Comment 17•20 years ago
|
||
|
|
||
Comment 18•20 years ago
|
||
Comment on attachment 194655 [details] [diff] [review]
patch (1.8)
> if (val) {
>- val.selected = true;
>- this.scrollBoxObject.ensureElementIsVisible(val);
> this.fireActiveItemEvent();
> }
remove single line brackets before checking this in.
|
|
Assignee | |
Comment 19•20 years ago
|
||
checked into trunk.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
|
|
||
Updated•20 years ago
|
Target Milestone: --- → mozilla1.8beta4
|
|
||
Comment 20•20 years ago
|
||
Comment on attachment 194655 [details] [diff] [review]
patch (1.8)
No known regressions so far, asking for approval.
Attachment #194655 -
Flags: approval1.8b4?
|
|
||
Updated•20 years ago
|
Attachment #194655 -
Flags: approval1.8b4? → approval1.8b4+
|
|
Assignee | |
Comment 21•20 years ago
|
||
checked into branch.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8
You need to log in
before you can comment on or make changes to this bug.
Description
•