activist-org / activist

An open-source activism platform
https://activist.org
GNU Affero General Public License v3.0
256 stars 201 forks source link

Expand a11y of CardTopicSelection #527

Closed andrewtavis closed 9 months ago

andrewtavis commented 1 year ago

Terms

Description

CardTopicSelection was finished in #276, with overall the component working really really well 😊 One final thing that was noted was that some accessibility improvements could be made. Specifically:

Note that the current implementation uses Headless UI combobox. Moving away from this should be done only if it's impossible to add this functionality via the Vue <script> block.

Contribution

Happy to support someone who has interest in working on this or get to it myself! 😊

andrewtavis commented 1 year ago

CC @bmartins-42 who worked on #276 :)

andrewtavis commented 1 year ago

Hey @bmartins-42 👋 Just talked this over with a coworker and we have a pretty good idea for this. Likely the best thing would have been to use an input and beneath it a Headless UI tabs component where the component is watching the input. This would allow us to skip from the text input to the options with a press of the tab key and then select options with left and right arrow keys. Beyond this the big thing is that apparently the tabs have an option for them being vertical, so we would just have this option enabled for mobile :)

Let's discuss and maybe I can work on this while you look at the modal!

TeddyGavi commented 12 months ago

@andrewtavis following up here from code night :) Happy to look into this :)

andrewtavis commented 12 months ago

Thanks @TeddyGavi! So happy to have you back and working on this in particular 😊

andrewtavis commented 10 months ago

5aac947 just fixes the styles as this is really proving to be difficult. What's missing:

Let me know if you have any thoughts on this, @TeddyGavi!

andrewtavis commented 10 months ago

I'm starting to wonder if it would be better if we build this from scratch 🤔 Seems like avoiding the default focus behavior of the tabs is gonna be really annoying.

UnknownSean8 commented 10 months ago

Hi @andrewtavis, bumping from #628, happy to help out, do you mind outlining the requirements for the issue?

Thanks :D

andrewtavis commented 10 months ago

Hey @UnknownSean8 👋 To outline the goals of this component so we can get on the same page for functionality:

I think that we're running into problems here as I incorrectly thought that the baseline functionalities of Headless UI Tabs could work here. Issue is that we're changing the order of things, and this is really screwing up the a11y that's built into the Headless UI component.

andrewtavis commented 10 months ago

Also we want this component to shift to a version where the options are flex-col below the md breakpoint so that we can have it work for mobile and further use this to filter search options as you saw with #597.

andrewtavis commented 10 months ago

Some further backstory here, this was originally a combobox like you worked on in #618, but that couldn't work for this as we weren't able to navigate between the options with left and right arrow keys (Headless UI combobox doesn't have an option to do left and right navigation instead of the default up and down).

UnknownSean8 commented 10 months ago

Ahh, got it, I'll look into it and let you know. Thanks, @andrewtavis

TeddyGavi commented 10 months ago

Hi @andrewtavis @UnknownSean8

FWIW I was also battling to override the default behaviour of the Headless UI Components.

I agree it could be simpler to write this by hand. Allowing more control over focus and keyboard navigation.

Looking forward to seeing the solution to this 😃

UnknownSean8 commented 10 months ago

Hi @andrewtavis @TeddyGavi,

I just looked into it just now.

Hey @UnknownSean8 👋 To outline the goals of this component so we can get on the same page for functionality:

  • We want an input and a list of topic options beneath it (done)

    • There are two alphabetically ordered lists in the topics: those that are selected followed by those that are unselected (done except ordering)
    • Topic options when selected should go to the selected part of the list (done)
  • We want the input to filter the topic options (done)

    • Once we've filtered to a level where we want to make a decision we should be able to tab into the topic options and use the arrow keys/return to select it (needed)
  • We want to be able to tab from the input and the first option in the topics and then use the arrow keys to navigate to the next ones - we don't want tab here as the full list is ~20 options that would be really annoying for someone to tab through (needed as it's not always the first one depending on selections)
  • We want to be able to tab from the topics back to the input (needed as sometimes we shift to the topic that was most recently selected first)
  • We want to be able to tab back to the topic options from the next item on the page (needed as it's not always the first one depending on selections)

I think that we're running into problems here as I incorrectly thought that the baseline functionalities of Headless UI Tabs could work here. Issue is that we're changing the order of things, and this is really screwing up the a11y that's built into the Headless UI component.

From this, I've tested several cases and it just gives me different results every time. Although I believe there should be some "tricks" to solve each requirement like the ComboBox, writing the component by hand would probably be easier and better fit to the requirements rather than trying to change the Headless UI Tab component itself. I think messing with the component itself will probably just make it less maintainable in the future.

Regardless, I will try out some implementations and let me know what are your thoughts, I would love to learn from both of you. :D

andrewtavis commented 10 months ago

Sounds like we're in agreement on a full rewrite for an amazing result! 😊 Here are some suggestions on structuring the work:

  1. We create a baseline with an input and two sets of buttons that are ordered appropriately where clicking them adds them to the other group and refilters
    • Includes all current styling
  2. We filter both button sets based on the input
  3. We prevent the normal tab functionality of the buttons in the sets where tabbing anywhere would go to the next page element and tabbing back would go to the input
  4. We add handlers to allow for left-right button navigation in case the sets are flex and up-down if they're flex-col
    • We'd need to factor for transitioning to the different set
    • Could maybe assign ids to them and navigate based on those?
    • Navigation would be with useMagicKeys no doubt
  5. Check for mobile design, with a final part being that for mobile we'd want to only display those that are selected with an option to open a dropdown with the unselected

How does this sound to the both of you? From there, @UnknownSean8, what would you like to work on? Happy to set the baseline for it and you can add the functionality (I'd do 1/2 above and maybe 5)?

UnknownSean8 commented 10 months ago

Sounds good, @andrewtavis. Thank you!

I'll try to implement 3/4 once the baseline is done.

Looking forward to it. :D

andrewtavis commented 10 months ago

Hey @UnknownSean8 and @TeddyGavi 👋 cb57435 reset the functionality of CardTopicSelection and removed the Headless UI components 😊 We're now done with steps 1, 2 and 5 from above, and are ready for 3 and 4! 🚀

Let me know how it's looking, and suggestions of course welcome!

UnknownSean8 commented 10 months ago

Hi @andrewtavis @TeddyGavi, I've added the functionality according to the checklist, but the up and down arrow is still pending.

It's working as expected, but IMO it's still not the best solution to the problem. Feel free to give any suggestions for the implementation.

Also, one problem I found is that the checkbox for the terms and conditions does not check when it's entered when focused.

Thanks!

andrewtavis commented 10 months ago

648 was just merged in 😊 @TeddyGavi, could you give the component a look over and let us know if you have any final comments on it? Would be great to get your final approval on this as you were the one who envisioned it in the first place :) :)

TeddyGavi commented 10 months ago

@andrewtavis will do! I've been following the conversation and fwiw this is much improved over the Headless Ui approach, in my opinion, good work @andrewtavis and @UnknownSean8 ! 👍

andrewtavis commented 9 months ago

Closed by #664 with a minor edge case being handled by #674 :) Thanks so much for all the hard work here, @UnknownSean8! 🎉 Really turned out so well 😊