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.

Sign up for GitHub

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

Jump to bottom

Fix a11y for repo topic management #23346

Closed
wants to merge 32 commits into from
Closed

Fix a11y for repo topic management #23346

wants to merge 32 commits into from

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 7, 2023 •
edited
Loading

Partially replace #22631

This PR improves the a11y for the Repo Topic Edit feature.

Thanks to @fsologureng for the idea and help.

Major changes

A11y for Fomantic Dropdown

extracted to a separate PR: #23450

Repo Topic Edit

  • Use <button> to replace <a>, then the Manage Topics button is focus-able and clickable (by keyboard).
  • Focus the dropdown input after click the Manage Topics button.
  • Fix UI misalignment, use flex
  • Fix Delete Icon misalignment, always use <svg>
  • Add "Cancel" button, the order is the same as Issue Title Edit (although I don't like this order)
  • Make the delete icon elements have correct aria-related attributes (mainly for mobile)

Now the Repo Topic Edit can be operated by keyboard-only.

  • Tab to the Manage Topics
  • Press Enter or Space to show the edit form
  • Use Left&Right Arrow keys to navigate between topics, press Backspace or Delete to delete them.
  • Input to load more topics, use Up&Down Arrow keys to select a topic
  • Press Tab to focus Cancel or Save

And mobile screen readers (like Talkback) could recognize the delete icon as a button.

Before

UI Misalignment & No Cancel Button

image

Delete Icon Misalignment

image

After

UI Aligned and Cancel Button

image

Delete Icon Aligned

image

@wxiaoguang wxiaoguang mentioned this pull request Mar 7, 2023
Closed
@techknowlogick techknowlogick added the topic/accessibility This issue/pull request wants to improve the accessibility label Mar 7, 2023
@techknowlogick techknowlogick added this to the 1.20.0 milestone Mar 7, 2023
@techknowlogick techknowlogick added type/bug topic/ui Change the appearance of the Gitea UI labels Mar 7, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 7, 2023
@wxiaoguang wxiaoguang marked this pull request as draft March 7, 2023 14:10
@wxiaoguang wxiaoguang marked this pull request as ready for review March 7, 2023 16:38
HesterG
HesterG reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@HesterG HesterG left a comment

Choose a reason for hiding this comment

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

Tested this case: If original topics are 'aaa' 'bbb' 'ccc' , and then click on "Manage topic", the aria-label for the delete svg on all three topics will become "Remove topic "aaa""

@wxiaoguang
Copy link
Contributor Author

Oops, my bad, fixed by 02c65fe

@wxiaoguang wxiaoguang force-pushed the fix-repo-topic-edit branch 2 times, most recently from 41c4972 to a868de8 Compare March 13, 2023 08:23
@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 13, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 13, 2023 •
edited
Loading

According to @fsologureng 's suggestion, always use "combobox", never use "menu" : 1fbff84

Just one-line change (isComboBox = true, with documents and comments).

update: reverted

@wxiaoguang wxiaoguang changed the title Fix accessibility behavior for topic management in the repository home, make some dropdown work as combobox Fix a11y for repo topic management, make all dropdowns as combobox Mar 13, 2023
@wxiaoguang wxiaoguang mentioned this pull request Mar 13, 2023
Merged
@wxiaoguang wxiaoguang marked this pull request as draft March 13, 2023 16:32
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 13, 2023 •
edited
Loading

To address some unclear (I don't understand) code-belonging concerns: #23450

The changes in 23450 are necessary for this PR. Without 23450, the dropdown doesn't work well on mobile.

@wxiaoguang wxiaoguang changed the title Fix a11y for repo topic management, make all dropdowns as combobox Fix a11y for repo topic management Mar 15, 2023
lunny pushed a commit that referenced this pull request Mar 17, 2023
@wxiaoguang
Fix aria.js bugs: incorrect role element problem, mobile focus proble…
345aa09 
…m, tippy problem (#23450)

This PR is extracted from #23346 to address some unclear (I don't
understand) code-belonging concerns.

This PR needs to be backported, otherwise the `aria.js` is too buggy in
some cases. Since there would be two minor conflicts, I will do the
backport manually.

Before: the `aria.js` is still buggy in some cases.

After: tested with AppleVoice, Android TalkBack

* Fix incorrect dropdown init code
* Fix incorrect role element (the menu role should be on the `$menu`
element, but not on the `$focusable`)
* Fix the focus-show-click-hide problem on mobile. Now the language menu
works as expected
* Fix incorrect dropdown template function setting
* Clarify the logic in aria.js
* Hide item's tippy after menu gets hidden
* Fix incorrect tippy `setProps` after `destroy`
* Fix UI lag problem when page gets redirected during menu hiding
animation with screen reader
* Improve comments
* Implement the layout proposed by #19861

<details>


https://github.com/go-gitea/gitea/blob/d74a7efb60f94a4b8e6e5f65332f94f1be31b761/web_src/js/features/aria.md?plain=1#L38-L47

</details>
@wxiaoguang wxiaoguang mentioned this pull request Mar 18, 2023
Merged
@silverwind
Copy link
Member

Which PR do you want to land first now, this one or #23542? I'm losing track.

@wxiaoguang
Copy link
Contributor Author

If I don't mention "block" or "merging order" in my PRs, then there is no order for them.

This PR will be discard because this PR caused some arguments about "attribution of ideas and code".

I will start new PRs from scratch, avoid touching unclear "idea" or "code".

@wxiaoguang wxiaoguang closed this Mar 20, 2023
@lunny lunny removed this from the 1.20.0 milestone Mar 21, 2023
@wxiaoguang wxiaoguang deleted the fix-repo-topic-edit branch March 22, 2023 04:07
@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.
Reviewers

@silverwind silverwind silverwind left review comments

@fsologureng fsologureng fsologureng left review comments

@HesterG HesterG HesterG approved these changes

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/accessibility This issue/pull request wants to improve the accessibility topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@wxiaoguang @codecov-commenter @HesterG @silverwind @fsologureng @lunny @techknowlogick @GiteaBot