Closed Bug 677276 Opened 10 years ago Closed 7 years ago

Numbering of access keys for attachments list in the menus should start at one, not zero

Categories

(Thunderbird :: Message Reader UI, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: srogers, Assigned: dorsatum, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 7 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

Opened a message containing an attachment.
Chose File > Attachments.



Actual results:

List of attachments appeared, numbered with keyboard shortcuts starting at zero.


Expected results:

Only programmers start counting things at zero. 
Ordinary users start counting things at one.
To be user-friendly and intuitive, the list of shortcuts for attachments should start at one and should not use zero.
Severity: normal → minor
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Numbering of keyboard shortcuts for Attachments list should start at one, not zero → Numbering of access keys for attachments list in the menus should start at one, not zero
Whiteboard: [good first bug]
(In reply to Stuart Rogers from comment #0)
> Chose File > Attachments.
> Only programmers start counting things at zero. 
> Ordinary users start counting things at one.
> To be user-friendly and intuitive, the list of shortcuts for attachments
> should start at one and should not use zero.

+1. Stuart, thanks for filing this little RFE. It's certainly something small, but nevertheless, it should be changed.
OS: Windows XP → All
Hardware: x86 → All
Version: 5.0 → Trunk
Blake, would you agree with this proposal of renumbering access keys for list of attachments in the menus to start from human-readable 1 rather than coder's 0? (File > Attachments)
If yes, could you indicate ui-r+ so that it's easier for volunteers to pick this up?
tia
Flags: needinfo?(bwinton)
There is nothing for me to ui-r, and without a working patch, a ui-r would be meaningless.
If someone wants to pick this up, they should feel free to.

Later,
Blake.
Flags: needinfo?(bwinton)
Probaner23 volunteered to take a stab at this in Bug 307824 Comment 11, following my hint and his successful fix for bug 307824 :)
Assignee: nobody → probaner23
Status: NEW → ASSIGNED
Whiteboard: [good first bug] → [good first bug][mentor=Aryx]
Hopefully this should resolve the issue regarding the index value of attachments.
Attachment #816306 - Flags: review?(archaeopteryx)
Comment on attachment 816306 [details] [diff] [review]
This patch aims to resolve the issue of attachments in Thunderbird starting with an index value of 0, instead of 1.

Review of attachment 816306 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.

::: mail/base/content/msgHdrViewOverlay.js
@@ +2454,5 @@
>    var indexOfSeparator = 0;
>    while (popup.childNodes[indexOfSeparator].localName != "menuseparator")
>      indexOfSeparator++;
>  
> +attachmentIndex+=1;

1) Please use the appropriate code indentation (2 whitespaces here, see adjacent lines).
2) The coding style also demands that there is a whitespace between operator ("+=") and other parts of the equation etc.
3) Please use ++ instead of +=1.

So the line should look like this:
  attachmentIndex++;

Increasing the argument passed to the function when passed/called in FillAttachmentListPopup is a good alternative.

Please fix 1) to 3) and submit a new patch.
Attachment #816306 - Flags: review?(archaeopteryx) → review-
Thank you Archaeopteryx for your feedback.
Attachment #816306 - Attachment is obsolete: true
Attachment #816381 - Flags: review?(archaeopteryx)
Thank you for your review Archaeopteryx, and I'm sorry for posting a similar patch in quick succession, I'd overlooked an error regarding the whitespace.
Attachment #816381 - Attachment is obsolete: true
Attachment #816381 - Flags: review?(archaeopteryx)
Attachment #816384 - Flags: review?(archaeopteryx)
Comment on attachment 816384 [details] [diff] [review]
THe previous patch had another issue with respect to whitespaces which has now been corrected.

Review of attachment 816384 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Because I am no official reviewer, I am requesting official review from mconley.
Attachment #816384 - Flags: review?(mconley)
Attachment #816384 - Flags: review?(archaeopteryx)
Attachment #816384 - Flags: feedback+
Thanks, Archaeopteryx.
> Looks good to me. Because I am no official reviewer, I am requesting
> official review from mconley.
Comment on attachment 816384 [details] [diff] [review]
THe previous patch had another issue with respect to whitespaces which has now been corrected.

Review of attachment 816384 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the caveat that an appropriate comment be inserted to explain the incrementing.

::: mail/base/content/msgHdrViewOverlay.js
@@ +2453,5 @@
>    // find the separator
>    var indexOfSeparator = 0;
>    while (popup.childNodes[indexOfSeparator].localName != "menuseparator")
>      indexOfSeparator++;
> +  attachmentIndex++;

We should add a comment explaining why we're incrementing this. Something like:

// We increment the attachmentIndex here since we only use it for the
// label and accesskey attributes, and we want the accesskeys for the
// attachments to be 1-indexed.
Attachment #816384 - Flags: review?(mconley) → review+
Attached patch ind2.patch (obsolete) — Splinter Review
Comments explaining the need to increase the index value added.
Attachment #820136 - Flags: review?(mconley)
Comment on attachment 820136 [details] [diff] [review]
ind2.patch

Review of attachment 820136 [details] [diff] [review]:
-----------------------------------------------------------------

Hi probaner,

I'll chip in with some feedback. As this already has review+ from mconley in comment 11 ("r=me with the caveat..."), there would normally be no need to request review from him again (we should use the precious time of our professionals sparingly ;). However as you're still new to patches, it's certainly helpful to request feedback again from volunteers like archaeopteryx or me.

feedback+ = me with the following nits fixed:

::: mail/base/content/msgHdrViewOverlay.js
@@ +2456,5 @@
>      indexOfSeparator++;
>    attachmentIndex++;
> +  // We increment the attachmentIndex here since we only use it for the
> +// label and accesskey attributes, and we want the accesskeys for the
> +// attachments to be 1-indexed.

Pls add the entire comment *above* the line of code which it refers to. Indent all comment lines with 2 spaces so as to left-align them nicely. Change last comment line to:

  // attachments list in the menu to be 1-indexed.

After that, optionally request feedback from either of us, and we'll add mconley's r+ flag back.
Attachment #820136 - Flags: review?(mconley) → feedback+
Probaner, also, if possible, could you add a picture showing screenshots of a sample attachments list old-style and after your patch side by side, I think that would ease and speed up ui-review a lot.
Thomas D. thanks a lot for your review! I'll submit a patch with these changes.
> feedback+ = me with the following nits fixed:
> 
> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +2456,5 @@
> >      indexOfSeparator++;
> >    attachmentIndex++;
> > +  // We increment the attachmentIndex here since we only use it for the
> > +// label and accesskey attributes, and we want the accesskeys for the
> > +// attachments to be 1-indexed.
Attached patch ind2.patch (obsolete) — Splinter Review
Patch fixes issue regarding alignment of comments and edits the last line of the comment.
Attachment #816384 - Attachment is obsolete: true
Attachment #820136 - Attachment is obsolete: true
Attachment #820824 - Flags: review?(bugzilla2007)
Comment on attachment 820824 [details] [diff] [review]
ind2.patch

Review of attachment 820824 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks probaner for the new patch.
I'm not an official reviewer, so it's better to ask for feedback.

There's a couple of nits that still need to be addressed...

Sorry I forgot one nit last time: The checkin comment (bug solution summary in your patch, "what the patch does") is too clumsy. It's fine to add some details in Bugzilla's description field of the patch (so that reviewers know what you've done), but the checkin comment should just state the general change done by this patch. The checkin comment will remain in the code repository for ever, so details like line numbers and comments don't belong there:

Bug 677276 - Start numbering of access keys for attachments list in the menus with 1, not 0

::: mail/base/content/msgHdrViewOverlay.js
@@ +2453,5 @@
>    // find the separator
>    var indexOfSeparator = 0;
> +  // We increment the attachmentIndex here since we only use it for the
> +  // label and accesskey attributes, and we want the accesskeys for the
> +  // attachments list in the menu to be 1-indexed.

That's correctly aligned now, but still in the wrong place. Pls note that your attachmentIndex++ is correctly NOT part of the while loop. So pls place the 3 comment lines directly above your new line having attachmentIndex++;. Preserve the existing blank line (no spaces!) after the line having indexOfSeparator++.

@@ +2458,4 @@
>    while (popup.childNodes[indexOfSeparator].localName != "menuseparator")
>      indexOfSeparator++;
>    attachmentIndex++;
> +  

Manually editing patches is generally not recommended, you lost the core line of your patch here which lacks a leading + to indicate it's new.
Also, no extra whitespace and no blank line here, pls.
Attachment #820824 - Flags: review?(bugzilla2007) → feedback-
This patch needs attachment 816384 [details] [diff] [review] (the patch which has r+) to apply. Please create a full patch with adding attachmentIndex++; and the comment directly above attachmentIndex++;
Attached patch att_ind.patch (obsolete) — Splinter Review
Does this solve all the errors?
Attachment #820824 - Attachment is obsolete: true
Attachment #821443 - Flags: feedback?(bugzilla2007)
Comment on attachment 821443 [details] [diff] [review]
att_ind.patch

Review of attachment 821443 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgHdrViewOverlay.js
@@ +2463,5 @@
>                        .getFormattedString("attachmentDisplayNameFormat",
>                                            [attachmentIndex, displayName]);
>    item.setAttribute("crop", "center");
>    item.setAttribute("label", label);
>    item.setAttribute("accesskey", attachmentIndex);

While we're here, could you please change this line to:

 item.setAttribute("accesskey", attachmentIndex % 10);

Right now, we're setting the accesskey wrong if you have more than 10 attachments. "10" is not a valid accesskey, but it'll "work" in the sense that it treats it the same as "1". Unfortunately, that means that if you had 19 attachments, there are 11 attachments whose accesskey is "1". If we take the last digit, it'll be easier to navigate around in cases like this.

(Yeah, I know, more than 10 attachments is pretty rare, but this is such an easy fix that I think it's worth doing anyway.)
Attached patch att_ind.patch (obsolete) — Splinter Review
Fixes issues related to accesskeys for more than 10 attachments
Attachment #821443 - Attachment is obsolete: true
Attachment #821443 - Flags: feedback?(bugzilla2007)
Attachment #821475 - Flags: review?(squibblyflabbetydoo)
Attachment #821475 - Flags: feedback?(bugzilla2007)
Comment on attachment 821475 [details] [diff] [review]
att_ind.patch

Looks good to me, though I didn't run with the patch or anything. r+!
Attachment #821475 - Flags: review?(squibblyflabbetydoo) → review+
(In reply to probaner23 from comment #21)
> Created attachment 821475 [details] [diff] [review]
> att_ind.patch
> 
> Fixes issues related to accesskeys for more than 10 attachments

This still needs attachment 816384 [details] [diff] [review] applied and now it's even worse and needs also attachment 820824 [details] [diff] [review].

Please make a full patch with all changes in one file.
This patch contains all changes. I'd had preferred if attachments no. 10 and higher don't get any accesskeys.
Attachment #821475 - Attachment is obsolete: true
Attachment #821475 - Flags: feedback?(bugzilla2007)
(In reply to Archaeopteryx [:aryx] from comment #24)
> Created attachment 821551 [details] [diff] [review]
> all-in-one patch, v1
> 
> This patch contains all changes. I'd had preferred if attachments no. 10 and
> higher don't get any accesskeys.

I'd believe users with accessibility issues (e.g. blind person with screenreader) would probably prefer to have accesskeys on *all* attachments (as seen in attachment 821551 [details] [diff] [review]):

With accesskeys on all attachments, you can jump around in steps of 10, which is considerably more efficient and precise than just going down the whole list using cursor etc. without access keys (and while you go down one by one, your screenreader will probably read them all if you're not fast enough).

Say with 33 attachments, you can press [1] subsequently to arrive precisely at the following indexed attachments:

1x[1] -> attachment  1 [details] [diff] [review]
2x[1] -> attachment 21 [details]
3x[1] -> attachment 31 [details] [diff] [review]

Same for pressing acceskey [2]

1x[2] -> attachment  2 [details] [diff] [review]
2x[2] -> attachment 22 [details]
3x[2] -> attachment 32 [details]
Comment on attachment 821551 [details] [diff] [review]
all-in-one patch, v1

Jim, could you pls set r+ again as you have already reviewed this 2 line patch.
Do we need ui-r for this obvious nitfix, or can we go straight to checkin-needed?
Attachment #821551 - Flags: review?(squibblyflabbetydoo)
I probably won't have time to do a full review of this until the weekend. While it looks sane and I'm sure it works, I still need to make sure it doesn't break any tests.
(In reply to Jim Porter (:squib) from comment #27)
> I probably won't have time to do a full review of this until the weekend.

Thank you Jim, I appreciate that.

> While it looks sane and I'm sure it works, I still need to make sure it
> doesn't break any tests.

If you could add some hints or links how to verify that this doesn't break tests, that would be a learning opportunity for me.
(In reply to Thomas D. (away till 23rd Oct) from comment #28)
> If you could add some hints or links how to verify that this doesn't break
> tests, that would be a learning opportunity for me.

You'd want to download the source code[1], apply the patch here, and either run the tests locally, or ideally get level 1 commit access[2] and push to the try server[3].

[1] https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build
[2] http://www.mozilla.org/hacking/commit-access-policy/
[3] https://wiki.mozilla.org/ReleaseEngineering/TryServer
Pushed the patch to Thunderbird-Try (only optimized builds and Mozmill tests): https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=cb5f5150972e
These take usually 3 hours to complete.
(In reply to Archaeopteryx [:aryx] from comment #31)
> Repushed after build system has been fixed:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=6f560905db8a

that's red and burning :(
Comment on attachment 821551 [details] [diff] [review]
all-in-one patch, v1

Review of attachment 821551 [details] [diff] [review]:
-----------------------------------------------------------------

The tests seem good, so r=me. I think we could probably improve the code in this area further, but that's beyond the scope of this bug.
Attachment #821551 - Flags: review?(squibblyflabbetydoo) → review+
Comment on attachment 821551 [details] [diff] [review]
all-in-one patch, v1

Bug summary (or comment 2) says it all.
Attachment #821551 - Flags: ui-review?(bwinton)
Comment on attachment 821551 [details] [diff] [review]
all-in-one patch, v1

(In reply to Jim Porter (:squib) from comment #34)
> Comment on attachment 821551 [details] [diff] [review]
> all-in-one patch, v1
> 
> Review of attachment 821551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests seem good, so r=me. I think we could probably improve the code in
> this area further, but that's beyond the scope of this bug.

Jim, perhaps you could just ui-r+ this 2-line patch, too, before it bitrots...
Also to appreciate the efforts of new patch contributor :dorsatum :)
Attachment #821551 - Flags: ui-review?(bwinton) → ui-review?(squibblyflabbetydoo)
Attachment #821551 - Flags: ui-review?(squibblyflabbetydoo) → ui-review+
Thanks, that was quick ;)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/42969141a750
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Mentor: archaeopteryx
Whiteboard: [good first bug][mentor=Aryx] → [good first bug]
Mentor: bugzilla2007
You need to log in before you can comment on or make changes to this bug.