Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep tab cycling inside modal when modal is active #3619

Merged
merged 8 commits into from
Apr 24, 2018

Conversation

aalmazan
Copy link
Contributor

Addresses #3558 with some ideas taken from https://stackoverflow.com/a/12444641/6553469.

Works well for me on both Chrome and Firefox. Have not had the chance to test IE11/Safari yet. I do wonder, though, if this should be within confirm_controller.js or in static/warehouse/index.js.

Anyway, let me know if there are any issues -- any feedback appreciated.

di
di previously requested changes Apr 12, 2018
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only seems to work when the "delete" button is enabled, otherwise it jumps out of the modal.

I also can't seem to get shift-tabbing to work, at least on Firefox (haven't tried Chrome).

Also it looks like we can't tab to the "Cancel" button, but that might be a separate issue.

I think we probably want to do what was suggested on the original issue, and bind event handlers to the first and last elements of the modals (and, second-to-last), rather than binding one event handler that gets every single keydown event.

@aalmazan
Copy link
Contributor Author

Interesting. This does work completely fine on my end even after rebasing upstream master and purging/rebuilding. Either way, indeed I glossed over that implementation detail of only catching the keypress on the first/last item and I'll update that.

Also it looks like we can't tab to the "Cancel" button, but that might be a separate issue.

While I can tab to "Cancel" on my end, using spacebar doesn't work properly (since it's an a and not a button element). Don't know if that's also an issue worth looking into.

@di
Copy link
Member

di commented Apr 13, 2018

Ah, realizing now this might be because of the way my Firefox browser is configured. I'll re-test this a bit more on Monday.

@nlhkabu nlhkabu requested review from nlhkabu and removed request for nlhkabu April 19, 2018 07:37
@nlhkabu nlhkabu self-assigned this Apr 19, 2018
@di di dismissed their stale review April 19, 2018 18:22

twas my fault

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I re-tested this, and appears to work great. Two things I think need changed before merging:

  • All of this JS should probably not be in the Stimulus controller, because it doesn't even use the Stimulus targets. Instead, let's put it inside a docReady in warehouse/warehouse/static/js/warehouse/index.js
  • We should make that cancel button a button and not a link, so that it can be "clicked" like the other button

@aalmazan
Copy link
Contributor Author

@di Great to know it works on your end too!

Also, I did some more fiddling around to bind the handlers directly on the first/second last/last elements, but think the approach above is the better one. Reason being: if we add handlers to the close button (the first item), SHIFT+TAB combinations when the button is selected will work, however, if you're on the text input and:

  1. (while on input field) hold SHIFT, press TAB
  2. (on close button) release TAB
  3. (still on close button) press TAB. This should be a SHIFT+TAB, but the SHIFT on the input field wasn't handled.

So it seems like we would have to add listeners to the entire modal or modal fields/buttons anyway?

@di
Copy link
Member

di commented Apr 20, 2018

Yes I agree, I think the current approach is fine.

@nlhkabu nlhkabu removed their assignment Apr 20, 2018
@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 20, 2018

Thank you for this @aalmazan - this is working great for me :)

@aalmazan
Copy link
Contributor Author

Updated with the requested changes:

  • Cancel now a button and its cancel action moved into confirm_controller.js
  • Moved tabbing handling into the main index.js file

I also added e73cd74 since I noticed the ESC handler was firing when modal elements exist on the page, but aren't active (yet). This was causing a null exception with my changes.

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what changed since I last reviewed, but it seems like I can no longer tab to the "Delete" button in any modal once it has been enabled by filling out the input.

@aalmazan
Copy link
Contributor Author

Sorry, my fault. Forgot to adjust the selectors that were used for cancel/confirm. This should good now.

@di di merged commit aa97808 into pypi:master Apr 24, 2018
@di di mentioned this pull request Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants