Improve attachmentlist navigation

RESOLVED FIXED in Thunderbird 15.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

Trunk
Thunderbird 15.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 617384 [details] [diff] [review]
Improve attachmentlist binding

I noticed some bugs in the attachmentlist code when working on my Attachment Tab add-on related to margins. Specifically, adding a margin screws up navigating with the keyboard. Here's a patch to fix it, and a couple other little issues I came across.

I suppose I can write tests, but I feel like Mozmill is the wrong way to test XBL bindings... any ideas?
Attachment #617384 - Flags: review?(bwinton)
Comment on attachment 617384 [details] [diff] [review]
Improve attachmentlist binding

>+++ b/mail/base/content/mailWidgets.xml
>@@ -67,60 +67,76 @@
>       <method name="getItemAtIndex">
>         <parameter name="index"/>
>         <body><![CDATA[
>-          return this.children[index] || null;
>+          if (index < this.children.length)

Should we also check for index >= 0?

Aside from that, it looks good.  r=me.
Attachment #617384 - Flags: review?(bwinton) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 623468 [details] [diff] [review]
Clean things up a bit more

Sorry to ask for review again, but looking at the code, I noticed that we were creating superfluous arrays because of the "children" property (whose name also conflicts with the member on DOM elements). I fixed these up, which should improve performance when there are lots of items in the attachment list.
Attachment #617384 - Attachment is obsolete: true
Attachment #623468 - Flags: review?(bwinton)
Comment on attachment 623468 [details] [diff] [review]
Clean things up a bit more

>+++ b/mail/base/content/mailWidgets.xml
>@@ -56,71 +58,90 @@
>       <method name="getItemAtIndex">
>         <parameter name="index"/>
>         <body><![CDATA[
>-          return this.children[index] || null;
>+          if (index >= 0 && index < this._childNodes.length)
>+            return this._childNodes[index];
>+          return null;
>         ]]></body>
>       </method>
>       <method name="getRowCount">
>         <body><![CDATA[
>-          return this.children.length;
>+          return this.itemCount;

Since we use this._childNodes.length up above, why not use it here, too?

Other than that, it seems fine, and passes all the tests, so r=me.

Thanks,
Blake.
Attachment #623468 - Flags: review?(bwinton) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)
> >+++ b/mail/base/content/mailWidgets.xml
> >@@ -56,71 +58,90 @@
> >       <method name="getItemAtIndex">
> >         <parameter name="index"/>
> >         <body><![CDATA[
> >-          return this.children[index] || null;
> >+          if (index >= 0 && index < this._childNodes.length)
> >+            return this._childNodes[index];
> >+          return null;
> >         ]]></body>
> >       </method>
> >       <method name="getRowCount">
> >         <body><![CDATA[
> >-          return this.children.length;
> >+          return this.itemCount;
> 
> Since we use this._childNodes.length up above, why not use it here, too?

Fixed in the patch I'm about to push...
(Assignee)

Comment 5

5 years ago
Checked in: http://hg.mozilla.org/comm-central/rev/d63ef5d1ea12
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.