Closed Bug 842356 Opened 7 years ago Closed 4 years ago

about:downloads in a tab: enable keyboard access (arrow keys, pg up/down)

Categories

(Firefox :: Downloads Panel, defect)

20 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: 326374, Assigned: n.parmar085, Mentored)

Details

Attachments

(1 file, 6 obsolete files)

Directly when the tab opens with about:downlaods it should be possible to
* select items with the up and down arrows
* scroll the page with the keyboard using page up/page down

Steps to reproduce
0. Download sufficiently many files that about:downloads will have more items than can be shown at once
1. Go to "about:downloads" in a new tab
2a. Press the up or down key to select a download (and later maybe later press the keyboard context menu button to remove a single download)
2b. Press the key "page down" to view more downloads

Actual result
* Nothing happens, because the scrollable area isn't focused

Expected result
* Expected a download to be selected or the page to scroll. Instead I have to tab to focus the area.

Ideally this should work like normal web pages do: having just the *window* focused is enough to enable page up/down and arrow keys. If that's difficult/impossible an alternative would be to automatically focus the <div> containing the downloads (well, actually the <richlistbox>). This must be done ever time that the tab gains focus, unless one of the elements childs already has focus.
looks like there is a focus problem indeed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I would like to work like to work on this bug. It will be my first.
Sure, you can ask questions to me or Paolo Amadini about this part of the code.
Since I didn't look into the possible causes for this bug yet, it may require some investigations and tries on your side to find the issue.
Assignee: nobody → n.parmar085
Mentor: mak77, paolo.mozmail
I've modified the ContentAreaDownloadsView.init() function to set focus to the downloads list it initializes before it returns. It seems to do the trick.
when you think the patch is ready, you can set the review flag on someone who reviewed code in this area already, if bugzilla doesn't suggest that, you can either look for reviewers on the hg log for the file you modified or, in case of a mentored bug, set the mentor as reviewer
Attachment #8680434 - Flags: review?(mak77)
Comment on attachment 8680434 [details] [diff] [review]
Modified contentAreaDownloadsView.js that now brings the downloads list in focus

Hi. Sorry but the format of the patch is wrong, it should be a diff against the current tree. I think this article can help, feel free to ask questions in case of doubts:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

The change itself looks good otherwise
Attachment #8680434 - Flags: review?(mak77)
Attached patch Bug842356_1.diff (obsolete) — Splinter Review
Exported previous fix as diff in mercurial.
Attachment #8680434 - Attachment is obsolete: true
Attachment #8682597 - Flags: review?(mak77)
Comment on attachment 8682597 [details] [diff] [review]
Bug842356_1.diff

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

It works fine. ideally it should focus the richlistbox only if there are downloads in the list, but since list population is async that would complicate the code just to support a not-so-common case (ideally one doesn't open about:downloads if he didn't download anything...).

Please add r=mak to the commit message.

There is a visual glitch I'd like to fix here though, the fact we hide the richlistbox border, but then show the focusring, looks inconsistent with other in-content UI and not very nice.
Thus I'd suggest to change in contentAreaDownloadsView.css:

#downloadsRichListBox:not(:-moz-focusring) {
   border-color: transparent;
}

with

#downloadsRichListBox:empty {
   border-color: transparent;
   background-color: transparent;
}

this will make so that we hide the border and the focusring when there are no downloads, but show it (either solid or focus ring) when there is any download. This way it should look more consistent with other in-content UI.

::: browser/components/downloads/content/contentAreaDownloadsView.js
@@ +10,5 @@
>      // Do not display the Places downloads in private windows
>      if (!PrivateBrowsingUtils.isContentWindowPrivate(window)) {
>        view.place = "place:transition=7&sort=4";
>      }
> +	//set focus to downloads list once it is created

please fix the indentation here and add a whitespace after //
Attachment #8682597 - Flags: review?(mak77) → feedback+
Attached patch bug842356_2.diff (obsolete) — Splinter Review
I just implemented both of your suggestions. While doing so, I also felt that making the focus ring around the downloads list box even when there are downloads looks quite nice (thus making it look like the download entry is the only element in focus, which seems more intuitive to me).

Let me know what you think!
Attachment #8682597 - Attachment is obsolete: true
Attachment #8684010 - Flags: review?(mak77)
Comment on attachment 8684010 [details] [diff] [review]
bug842356_2.diff

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

::: browser/components/downloads/content/contentAreaDownloadsView.css
@@ +11,5 @@
>  }
> +
> +#downloadsRichListBox{
> +	border-color: transparent;
> +}

Unfortunately, while it looks better, this is negative for accessibility (if you open about downloads and keep pressing TAB you'll see what I mean) and it's not consistent with other in-content ui.

Rather please remove downloadsRichListBox:not(:-moz-focusring) and replace it with the new rule, as I suggested
Attachment #8684010 - Flags: review?(mak77) → feedback-
note: please pay attention to indentation.
Done!
Attachment #8684010 - Attachment is obsolete: true
Attachment #8685512 - Flags: review?(mak77)
Comment on attachment 8685512 [details] [diff] [review]
Removed lines that made dowloads list box focus ring transparent.

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

sorry, this patch is built on top of the previous one and it's still not removing downloadsRichListBox:not(:-moz-focusring) ... could you please merge the patches?

The final patch should apply just on top of the current tree...
If you're having difficulties I can eventually do that for you.
Attachment #8685512 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #13)
> Comment on attachment 8685512 [details] [diff] [review]
> Removed lines that made dowloads list box focus ring transparent.
> 
> Review of attachment 8685512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sorry, this patch is built on top of the previous one and it's still not
> removing downloadsRichListBox:not(:-moz-focusring) ... could you please
> merge the patches?
> 
> The final patch should apply just on top of the current tree...
> If you're having difficulties I can eventually do that for you.

I'm sorry about the diff, I'll export a patch against the current tree right away.

However, I'm not sure what you mean about the downloadsRichListBox:not(:-moz-focusring). These are the contents of contentAreaDownloadsView.css from dxr:

> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> #downloadsListEmptyDescription {
>   display: none;
> }
> 
> #downloadsRichListBox:empty + #downloadsListEmptyDescription {
>   display: -moz-box;
> }
> This page was generated by DXR 2015-11-10 10:34.
> 

The lines you mentioned do not exist. I have added the lines you required, which hides the background and focus ring when there are no downloads. Am I missing something? this is my first time with CSS.
Ah sorry, I'm looking at browser/themes/shared/downloads/contentAreaDownloadsView.css
while you are looking at browser/components/downloads/content/contentAreaDownloadsView.css

your changes should go into browser/themes/shared/downloads/contentAreaDownloadsView.css cause they are visual, not functional.
Stuff in theme is visual and can be overridden by themes, while stuff in content is functional and it's needed for proper functionality.
Attachment #8685512 - Attachment is obsolete: true
Attachment #8686417 - Attachment is obsolete: true
Attachment #8686417 - Attachment description: bug842356_4.diff - diff against current tree and modified correct file. → bug842356_4.diff - modified correct file.
Attachment #8686417 - Attachment is obsolete: false
Attached patch bug842356.patch (obsolete) — Splinter Review
Attachment #8686445 - Attachment is obsolete: true
Attached patch bug842356.patchSplinter Review
I'm not sure how to diff against the current tree...I've tried exporting a local commit as a diff file, as well as using a queue to generate a patch file. The attachment always shows a diff against the previous attachment...
Flags: needinfo?(mak77)
Comment on attachment 8686452 [details] [diff] [review]
bug842356.patch

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

Yep, this looks good. thank you!
Attachment #8686452 - Flags: review+
Attachment #8686417 - Attachment is obsolete: true
Flags: needinfo?(mak77)
(In reply to Nicholas Parmar from comment #18)
> Created attachment 8686452 [details] [diff] [review]
> bug842356.patch
> 
> I'm not sure how to diff against the current tree...I've tried exporting a
> local commit as a diff file, as well as using a queue to generate a patch
> file. The attachment always shows a diff against the previous attachment...

if you're using mozilla queues you can just use hg qfold to merge patches, if instead you are using commits you can use histedit to fold them.

You can find more information here (old process):
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial
or here (new mozreview process):
http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html
https://hg.mozilla.org/mozilla-central/rev/ea1029216415
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
I have reproduced this bug with Firefox nightly(build id-20130218031106)on
windows 7(64 bit)

I have verified as fixed this bug with Firefox aurora 45.0a2(build id:20160114004007)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

[testday-20160115]
You need to log in before you can comment on or make changes to this bug.