mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
126 stars 41 forks source link

Flag review menu does not close when navigating it with a keyboard #2029

Open vcarciu opened 6 years ago

vcarciu commented 6 years ago

Steps to reproduce: 1.Go an add-on detail page 2.Click on Read all reviews to see the list of reviews 3.When the page loads, use only the tab key to navigate the page 4.When you tab to the flag link press the space bar or enter key to flag a review 5.When drop down menu is collapsed, navigate through options by pressing Tab on keyboard

Expected results: When reaching the last option, one more Tab press should focus the first option on the drop down(basically user should stay inside the opened drop down menu )

Actual results: The next option on site is selected and user is not able to go back in the drop down menu without going through entire site with the keyboard

NOTES: Reproduced in AMO-dev, FF Release

Please see video for this issue : navigationindropdown

┆Issue is synchronized with this Jira Task

vcarciu commented 6 years ago

Derived from mozilla/addons#10980

seanprashad commented 5 years ago

I think we'd need to look into using tabindex or focus().

Do we want the user to be able to cycle through the 3 options or simply close the menu if they tab enough times to move off the final option?

Code in question can be found here:

https://github.com/mozilla/addons-frontend/blob/06b8556c2ecd33f7d2c68f7692994e809c16a42d/src/ui/components/TooltipMenu/index.js#L45

kumar303 commented 5 years ago

Yes, possibly. If you look at the DOM, the flag menu is buried at the bottom (iirc) which is part of the problem.

seanprashad commented 5 years ago

What Kumar is referring to: https://github.com/mozilla/addons-frontend/blob/2d0ba6219d6e213822d192991026f530ffe70b0e/src/amo/components/AddonReviewCard/index.js#L452-L456

https://github.com/mozilla/addons-frontend/blob/2d0ba6219d6e213822d192991026f530ffe70b0e/src/amo/components/FlagReviewMenu/index.js#L73

seanprashad commented 5 years ago

I was looking into using focus() but after making this small codepen, it isn't cycling through each button as I hoped..

Just curious @kumar303 - you mentioned that FlagReviewMenu is "buried" at the bottom - should I look to actually move that around somewhere else?

Logically, we need to have the tabindex set such that button 1 will be before button 2, and button 2 will be before button 3, BUT button 3 will point to button 1's tabindex - still investigating alternatives to focus()

kumar303 commented 5 years ago

should I look to actually move that around somewhere else?

Well, I think that would be hard because this was the way that the third party component was implemented.

ani-sha commented 4 years ago

Hi @vcarciu @kumar303, I am an Outreachy participant for May 2020. I would like to work on this issue.

valkyr13 commented 4 years ago

@vcarciu @kumar303 I'm an Outreachy applicant, can I work on this issue?

kumar303 commented 4 years ago

Hi, thanks for your interest. @muffinresearch maybe you could help coordinate?

muffinresearch commented 4 years ago

@ani-sha, @valkyr13 this issue is probably not the best place to start, maybe take a look at something from this list https://github.com/mozilla/addons-frontend/issues?q=is%3Aopen+is%3Aissue+label%3A%22contrib%3A+outreachy%22

avgupt commented 4 years ago

Can I take up this issue?

bobsilverberg commented 4 years ago

@avgupt I see you have another PR in progress, and we think it best that you not work on more than a couple of issues at a time. You can take this on, but looking at it it looks pretty tricky to me, and I cannot with confidence say that we'll be able to give you much help in solving it. Please feel free to give it a try, but it might not be an ideal issue for your purposes.

avgupt commented 4 years ago

Hey @bobsilverberg , I gave it a shot but the tooltip that's being used is a third party component. I'm just stuck at how I could trigger a close command for tooltip, when the user presses Tab after reaching the third row. I'll give it a try in sometime, but for now I'm unassigning myself. If you have any ideas please let me know.

shribyte commented 1 year ago

I've been working on an implementation for the bug.

The idea is to add React refs for the first and last list items in the items prop (will be used to control the focus when navigating the dropdown via keyboard) Then utilize a handleKeyDown event handler that allows looping through the menu (loops to the first item after the last one)

Would love your feedback!

shribyte commented 1 year ago

I do realize that the third party component RCTooltip makes it not so easy to close the menu. Welcome any suggestions here.

I don't see suggestions regarding this in parallel efforts such as this.

One way out may be replacing the third party tooltip, but this doesn't seem to be the most efficient approach.

For now, I will be pushing changes that allow looping the menu, however, this isn't ideal given that we wouldn't be able to break out of the menu.

KevinMind commented 5 months ago

Old Jira Ticket: https://mozilla-hub.atlassian.net/browse/ADDFRNT-123