Closed
Bug 747824
Opened 12 years ago
Closed 12 years ago
Improve attachmentlist navigation
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 15.0
People
(Reporter: squib, Assigned: squib)
Details
Attachments
(1 file, 1 obsolete file)
|
12.15 KB,
patch
|
bwinton
:
review+
|
Details | Diff | Splinter Review |
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 1•12 years ago
|
||
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•12 years ago
|
||
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 3•12 years ago
|
||
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•12 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•12 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/d63ef5d1ea12
Status: ASSIGNED → RESOLVED
Closed: 12 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.
Description
•