richlistbox needs to use template listeners

VERIFIED FIXED in mozilla1.8beta4

Status

()

Toolkit
XUL Widgets
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Doron Rosenberg (IBM))

Tracking

({fixed1.8})

unspecified
mozilla1.8beta4
x86
Windows XP
fixed1.8
Points:
---
Bug Flags:
blocking1.8b4 -
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

13 years ago
See my backed out patch in https://bugzilla.mozilla.org/show_bug.cgi?id=305667
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

13 years ago
Created attachment 194437 [details] [diff] [review]
1.8 branch patch

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

13 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

13 years ago
Created attachment 194441 [details] [diff] [review]
with irc comments addressed (still 1.8 only)
Attachment #194437 - Attachment is obsolete: true
Attachment #194441 - Flags: first-review?(bugs.mano)
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-
(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 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

13 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

13 years ago
Created attachment 194538 [details] [diff] [review]
with comments addressed
Attachment #194441 - Attachment is obsolete: true
Attachment #194538 - Flags: first-review?(bugs.mano)
Comment on attachment 194538 [details] [diff] [review]
with comments addressed

+      <field name="_builderListener">
>+        <![CDATA[
>+          ({
>+            mOuter: this,
>+            item: null,
>+            willRebuild : function(builder) {},
>+            didRebuild : function(builder) {
>+              this.mOuter._refreshSelection()

missing semicolon here.

>+            }
>+          })

another one


>+      <method name="_refreshSelection">
>+      <body>
>+        <![CDATA[
....
>+          // happens.
>+          if (this.selectedItem) {
>+            if (this.selectedItem.hasAttribute("id")) {
>+              var id = this.selectedItem.getAttribute("id");
>+              var item = document.getElementById(id);
>+  

Please add a comment here explaining that the orginal item is gone regardlees.


>+          // if we have no previously selected item or the above if check fails to
>+          // find the previous node (which causes it to clear selection)
>+          if (!this.selectedItem) {
>+            // if the selectedIndex is larger than the row count, select the last
>+            // item.
>+            if (selectedIndex >= this.getRowCount())
>+              this.selectedIndex = this.getRowCount() - 1;
>+            else
>+              this.selectedIndex = selectedIndex;
>+          }
...
>+      <field name="_selectedIndex">-1</field>

So, it looks like the first time you open a richlistbox, |_refreshSelection|
will try to set the selectedIndex to -1, then the first <richlistitem> ctor
selects the first item. How about changing the |_selectedIndex| field initial
value to 0 and remvoing the richlistitem ctor part?

>       <property name="selectedIndex">
>         <getter>
>           <![CDATA[
>@@ -121,6 +203,9 @@
>         </getter>
>         <setter>
>           <![CDATA[
>+            if (val == this._selectedIndex)
>+              return;
>+

I'm in favor of removing this optimization, unless this is really desired.

Please rediff with -u8.
Attachment #194538 - Flags: first-review?(bugs.mano) → first-review-
(Please also fix the missing semicolon in the selectedIndex setter).

Comment 12

13 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

13 years ago
Created attachment 194578 [details] [diff] [review]
third time's a charm

I tested this pretty decently and all seems well.
Attachment #194538 - Attachment is obsolete: true
Attachment #194578 - Flags: first-review?(bugs.mano)
(Assignee)

Updated

13 years ago
Attachment #194578 - Attachment is obsolete: true
Attachment #194578 - Flags: first-review?(bugs.mano)
(Assignee)

Comment 14

13 years ago
Created attachment 194597 [details] [diff] [review]
puts back the last-selected setting code into richlistbox, 1.8 only still
Attachment #194597 - Flags: first-review?(bugs.mano)
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

13 years ago
Created attachment 194655 [details] [diff] [review]
patch (1.8)

I had to keep the final ensureSelectedElementIsVisible() because it works
around a bug in scrollbox, I added an XXX: comment.
(Assignee)

Comment 17

13 years ago
Created attachment 194657 [details] [diff] [review]
trunk patch
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

13 years ago
checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta4
Comment on attachment 194655 [details] [diff] [review]
patch (1.8)

No known regressions so far, asking for approval.
Attachment #194655 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #194655 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 21

13 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.