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

Use a general approch to improve a11y for all checkboxes and dropdowns. #23542

Merged
merged 5 commits into from Mar 22, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 17, 2023

This PR follows #22599 and #23450

The major improvements:

  1. The aria-*.js are totally transparent now, no need to call attachDropdownAria explicitly anymore.
    • It hooks the $.fn.checkbox and $.fn.dropdown, then our patch works.
    • It makes all dynamically generated checkbox/dropdown work with a11y without any change
    • eg: the conversation.find('.dropdown').dropdown(); in repo-diff.js
  2. Since it's totally transparent now, it could be easier to modify or remove in the future.
  3. It handles all selection labels as well (by onLabelCreate), so it supports "multiple selection dropdown" now.
    • It partially completes one of my TODOs: TODO: multiple selection is not supported yet.
  4. The code structure is clearer, code blocks are splitted into different functions.
    • The old attachOneDropdownAria was splitted into separate functions.
    • It makes it easier to add more fine tunes in the future, and co-work with contributors.
  5. The code logic is similar as before, only two new parts:
    1. the ariaCheckboxFn and ariaDropdownFn functions
    2. the onLabelCreate and updateSelectionLabel functions

In aria-dropdown.js I had to mix jQuery and Vanilla JS somewhat, I think the code is still understandable, otherwise the code would be much more complex to read.

Thanks to fsologureng for the idea about "improving the 'delete icon' with aria attributes".

If there is anything unclear or incorrect, feel free to ask and discuss, or propose new PRs for it.

@wxiaoguang wxiaoguang force-pushed the improve-dropdown-a11y branch 3 times, most recently from f4ed870 to 25b702c Compare March 17, 2023 12:39
@silverwind silverwind added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Mar 17, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 18, 2023
@silverwind
Copy link
Member

silverwind commented Mar 19, 2023

For checkbox, I wonder if it wouldn't be better to move to native checkboxes instead (removing the fomantic module). It'll be the best solution and a11y is perfect for those and they don't need any extra JS work.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 20, 2023

For checkbox, I wonder if it wouldn't be better to move to native checkboxes instead (removing the fomantic module). It'll be the best solution and a11y is perfect for those and they don't need any extra JS work.

Agree, but it really needs much more time, including implement the necessary CSS styles, rewrite related $().checkbox() code.

I have done my best to avoid the checkboxes becoming more complex, to make the future native-replacing not too difficult.

@silverwind
Copy link
Member

I think I will get started on native checkbox styling as part of an effort the eliminate the checkbox "pop in" on the issue list. It's been annoying me for a while now.

@silverwind
Copy link
Member

See #23596 for that. I think I will add styling, currently there is none.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 20, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 21, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #23542 (a84a9ef) into main (f521e88) will decrease coverage by 0.02%.
The diff coverage is 37.89%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23542      +/-   ##
==========================================
- Coverage   47.14%   47.13%   -0.02%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152353     +907     
==========================================
+ Hits        71397    71808     +411     
- Misses      71611    72057     +446     
- Partials     8438     8488      +50     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/context/context.go 64.54% <0.00%> (-3.53%) ⬇️
modules/doctor/storage.go 31.93% <0.00%> (ø)
... and 33 more

... and 38 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 22, 2023
@lunny lunny merged commit d4f35bd into go-gitea:main Mar 22, 2023
2 checks passed
@wxiaoguang wxiaoguang deleted the improve-dropdown-a11y branch March 22, 2023 03:12
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 22, 2023
* upstream/main:
  Use a general approch to improve a11y for all checkboxes and dropdowns. (go-gitea#23542)
  [skip ci] Updated translations via Crowdin
  Update PR documentation (go-gitea#23620)
  Set opaque background on markup and images (go-gitea#23578)
  Decouple the issue-template code from comment_tab.tmpl  (go-gitea#23556)
  Remove `id="comment-form"` dead code, fix tag (go-gitea#23555)
  Introduce path Clean/Join helper functions (go-gitea#23495)
  Remove conflicting CSS rules on notifications, improve notifications table (go-gitea#23565)
  Remove @metalmatze as maintainer (go-gitea#23612)
  Keep (add if not existing) xmlns attribute for generated SVG images (go-gitea#23410)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants