1702694 - about downloads - Shift the section highlight focus over on the list elements

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P3)

Description

[Cristian Fogel [:cfogel]]

Attachment: abDownloads.png

Affected versions

• 88.0b3, 89.0a1(2021-03-31);

Affected platforms

• Windows 10, Ubuntu 20;

Steps to reproduce

1. Launch Firefox, download 2+test/temp files
2. Access about:downloads;
3. Press the TAB key or click on any listed download in the page;

Expected result

• highlight applied on listed element;

Enhancement suggestion

• a good UX reference might be about:addons - theme section / hover + tab-ing over themes

Actual result

• while the highlight is indeed applied on the listed elements

Regression range

• initially, the focus consisted of the dotted-line surrounding the list area, that wasn't "Stealing" focus from the listed elements (even as it was colored blue .... so considering that as "good":
• First bad: 2019-09-25;
• Last good: 2019-09-24;
• Pushlog: URL ;

Additional notes

• while this is not Proton specific, it's something that might be improved now;
• attached screenshot to illustrate the suggestion, to be more consistent across the browser areas.

Comment 1

[Vaidehi]

Hello, this is Vaidehi, an outreachy applicant. Looking for my first bug. Can I work on this one?

Comment 2

[:Gijs (he/him)]

It's not really clear to me what the desired behaviour here is supposed to be. The focus ring appears on the list when the document is loaded initially, but not when you focus the address bar and then the document again.

Vaidehi, you're welcome to work on this but we'd need to not break the focus ring in general - I suspect we would just want to ensure that whatever is focusing the tree is passing focus options that avoid triggering the focus ring. Perhaps at https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/allDownloadsView.js#496 ? But I don't know for sure if that's the right place, you would have to investigate.

Comment 3

[:Gijs (he/him)]

(You can avoid showing the focus ring with .focus() calls like so: https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/downloads.js#1501 )

Comment 4

[Vaidehi]

(In reply to :Gijs (he/him) from comment #2)

It's not really clear to me what the desired behaviour here is supposed to be. The focus ring appears on the list when the document is loaded initially, but not when you focus the address bar and then the document again.

Vaidehi, you're welcome to work on this but we'd need to not break the focus ring in general - I suspect we would just want to ensure that whatever is focusing the tree is passing focus options that avoid triggering the focus ring. Perhaps at https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/allDownloadsView.js#496 ? But I don't know for sure if that's the right place, you would have to investigate.

Okay, will work on it and let you know.

Comment 5

[Vaidehi]

I think here, the reporter wants to remove the focus ring of the entire list and let the highlight and focus be only for individual list elements(when they are clicked).
But Gijs, you think that the list focus ring shouldn't be removed, and should be visible even after clicking on the adress bar. Correct me if I am wrong.
And, then there's scope for focusing on hovering.

Comment 6

[Vaidehi]

(In reply to :Gijs (he/him) from comment #3)

(You can avoid showing the focus ring with .focus() calls like so: https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/downloads.js#1501
The comments at the beginning of "downloads.js" file describes the focus and focus rings behavior. Still, I am not sure what exactly should be done here.

Comment 7

[:Gijs (he/him)]

(In reply to Vaidehi from comment #6)

(In reply to :Gijs (he/him) from comment #3)

(You can avoid showing the focus ring with .focus() calls like so: https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/downloads.js#1501
The comments at the beginning of "downloads.js" file describes the focus and focus rings behavior. Still, I am not sure what exactly should be done here.

We should ensure that the initial focusing of the listbox does not show a focus ring. For this you will need to find out where we focus the listbox automatically, causing the focusring to be shown. I suspect this is the line I linked, ie https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/allDownloadsView.js#496 . A first step would be commenting it out and seeing if that makes the problem go away.

Comment 8

[Vaidehi]

So, I commented out this line -
https://searchfox.org/mozilla-central/source/browser/components/downloads/content/contentAreaDownloadsView.js#18
and the focus ring of the listbox is completely gone. Have attached the screenshot too. What should we do next?

Comment 9

[Vaidehi]

Attachment: Focus ring not seen when page is loaded initially as well as after any cursor movement

Comment 10

[:Gijs (he/him)]

(In reply to Vaidehi from comment #8)

So, I commented out this line -
https://searchfox.org/mozilla-central/source/browser/components/downloads/content/contentAreaDownloadsView.js#18
and the focus ring of the listbox is completely gone.

The focus ring won't be gone if you use tab/shift-tab.

Have attached the screenshot too. What should we do next?

I explained this in comment #2 and comment #3. Now that you have found the right focus invocation, you can use those to fix the bug.

Comment 11

[Vaidehi]

Hello Gijs.
I thought, https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/allDownloadsView.js#921 , would be the right place for using preventFocusRing, after going through all the files. But it did not work. I also tried using "outline:none" in css and adding listeners for keyup and keydown events. But none of them changes the default tabindex sequence focusring on the richlistbox.
So, I would request you to guide me from here, if you have time, or may be assign another bug considering the nearing deadline of contribution period.
Thanks.

Comment 12

[:Gijs (he/him)]

(In reply to Vaidehi from comment #11)

Hello Gijs.
I thought, https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/allDownloadsView.js#921 , would be the right place for using preventFocusRing, after going through all the files. But it did not work. I also tried using "outline:none" in css and adding listeners for keyup and keydown events. But none of them changes the default tabindex sequence focusring on the richlistbox.

We don't want to change the focus order or tabindex order.

All we want to do is, when the page loads, not to show a focus outline immediately around the entire list box.

For this, adjusting the focus call in comment #10 should be sufficient. Did you try this?

Comment 13

[Vaidehi]

(In reply to :Gijs (he/him) from comment #12)

(In reply to Vaidehi from comment #11)

Hello Gijs.
I thought, https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/browser/components/downloads/content/allDownloadsView.js#921 , would be the right place for using preventFocusRing, after going through all the files. But it did not work. I also tried using "outline:none" in css and adding listeners for keyup and keydown events. But none of them changes the default tabindex sequence focusring on the richlistbox.

We don't want to change the focus order or tabindex order.

All we want to do is, when the page loads, not to show a focus outline immediately around the entire list box.

For this, adjusting the focus call in comment #10 should be sufficient. Did you try this?

Yes, I tried it. Using preventFocusRing doesn't show the listbox ring/outline when the page loads initially. But when I use tab key and traverse through different components and again reach the downloads list, the ring appears. This is what you pointed out in comment 10. So, that's where I am stuck.
I have used preventFocusRing here - https://searchfox.org/mozilla-central/source/browser/components/downloads/content/contentAreaDownloadsView.js#18
Thanks.

Comment 14

[:Gijs (he/him)]

(In reply to Vaidehi from comment #13)

Using preventFocusRing doesn't show the listbox ring/outline when the page loads initially.

Great.

But when I use tab key and traverse through different components and again reach the downloads list, the ring appears. This is what you pointed out in comment 10.

That's what we want! When using tab/shift-tab, the focus ring should appear. It just shouldn't be there automatically when the document opens.

Comment 15

[Vaidehi]

(In reply to :Gijs (he/him) from comment #14)

(In reply to Vaidehi from comment #13)

Using preventFocusRing doesn't show the listbox ring/outline when the page loads initially.

Great.

But when I use tab key and traverse through different components and again reach the downloads list, the ring appears. This is what you pointed out in comment 10.

That's what we want! When using tab/shift-tab, the focus ring should appear. It just shouldn't be there automatically when the document opens.

Oh..okay.
It was really my mistake. I wasted time on doing something which wasn't required. Sorry for the delay.
So, what can I do next? Submit a patch?
Thanks.

Comment 16

[:Gijs (he/him)]

(In reply to Vaidehi from comment #15)

So, what can I do next? Submit a patch?

Yes, please submit a patch.

Comment 17

[Vaidehi]

Attachment: Bug 1702694 - Use preventFocusRing option to disable the focus ring of downloads list on loading. r=Gijs

Comment 18

[Vaidehi]

Is the patch alright?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddcdbe14a718
Use preventFocusRing option to disable the focus ring of downloads list on loading. r=Gijs

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/ddcdbe14a718

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:b19208, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[Vaidehi]

Comment on Attachment #9218403 [details]
Bug 1702694 - Use preventFocusRing option to disable the focus ring of downloads list on loading. r=Gijs

Beta/Release Uplift Approval Request -

User impact if declined: User will see focus ring on the downloads list which would compromise the focus of individual elements
Is this code covered by automated/manual tests?: No
Risk in taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Small Javascript fix to just hide focus ring once on loading

Comment 23

[Vaidehi]

Comment on attachment 9218403 [details]
Bug 1702694 - Use preventFocusRing option to disable the focus ring of downloads list on loading. r=Gijs

Beta/Release Uplift Approval Request

• User impact if declined: User will see focus ring on the downloads list which would compromise the focus of individual elements
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small Javascript fix to just hide focus ring once on loading
• String changes made/needed: no

Comment 24

[Pascal Chevrel:pascalc]

Comment on attachment 9218403 [details]
Bug 1702694 - Use preventFocusRing option to disable the focus ring of downloads list on loading. r=Gijs

We have had this regression since Firefox 71, I think that the fix can ride the 90 train, thanks.

Comment 25

[Giorgia Nichita, Release Desktop QA]

I have reproduced the issue on an affected Nightly build 89.0a1 (2021-04-01)
The fix was verified on 90.0b2 (2021-06-01). Tests were performed on Ubuntu 20.04, Windows 10 and macOS 10.15.
Thanks!