Remove customized (unmaintained) dropdown, improve aria a11y for dropdown

[wxiaoguang]

Background

According to #8638 (comment) , the customized dropdown is unmaintained.

To make the code maintainable again, this PR removes that customized dropdown, and use a general method to make a11y work with Fomantic UI dropdown.

There are some problems on Fomantic UI side, so the a11y support is not perfect (see the comment), I think it's still better than before (compared to https://try.gitea.io/repo/create), and there will be no blocker to follow the latest Fomantic UI releases.

Close #10672, Close #16581

• Remove/Replace custom version of semantic dropdown #10672
• Reintegrate the dropdown patch using fomantic hooks and template changes #16581

Some explanations

1. The initRepoSettingsCollaboration becomes more complex: because it has to behave correctly with aria/a11y. Before this PR, the behavior was not correct, this PR considers more (maybe all) edge cases.
2. The changes in _form.less: before this PR it made the inputs inside the dropdown half-width, which was incorrect. This PR fixes it.

TODOs

Since I am not expert for aria, if there is anything incorrect or to be improved, please edit on this PR directly or propose PRs for it. Feel free to make it more correct.

Screenshots

The best page to test is http://localhost:3000/repo/create (compared to https://try.gitea.io/repo/create)

[Jookia]

I did some testing of this using Orca with Chromium. Some notes:

• Overall everything seems fine, and the code looks a lot better
• Icons are not set to tabindex -1 so you can focus menu buttons twice
• You removed the call to .click() so now you can't set repo collaborator permissions
• Combobox/option is probably not the correct role for this? I think listbox fits better.

[Flameborn]

Thanks for creating this PR. Dropdowns were Tested via VoiceOver under MacOS and iOS, and NVDA and Jaws under Windows, the functionality is almost identical to the old patch, which is definitely great to see.

The tabIndex and the .click() method are likely the two things that need a fix.

Regarding the role, I can't think of a case when a listbox would be better, as the usage from a screen reader user's standpoint is almost identical. It might be, however, that for filtering reasons, specific pages will need a specific role, but I think this can be addressed in later PRs, when needed.

[wxiaoguang]

I need some more details about some points:

Icons & tabindex & focus

Which icon? Can you help to provide the page/DOM/element for it? I have difficulty to reproduce the focus-twice problem ....

The click() and repo collaborator permissions

I think the problem might be fixed on the collaborator permissions page, instead of changing the dropdown's behavior. I will look into it later.

Fixed on the repo perssmion page. Indeed it's a misuse of dropdown.

role=combobox/option

The difference between combobox and menu:

• combobox has a text field and a option list. If the option is changed, the text field is also updated.
• menu has a button and a menu list, the menu item only triggers actions, doesn't change the menu button.

So, in my mind that's the proper roles, because the dropdown behaves like a combobox (but in some cases it is used as a menu).

[Jookia]

On Wed, Jun 01, 2022 at 04:39:10AM -0700, wxiaoguang wrote: Which icon? Can you help to provide the page/DOM/element for it? I have difficulty to reproduce the focus-twice problem ....

Sorry, my mistake: What's happening is that once the menu opens, the next tab is to the first menu item instead of the next menu. Once you tab the menu closes and you're lost.

> The click() and repo collaborator permissions I think the problem might be fixed on the collaborator permissions page, instead of changing the dropdown's behavior. I will look into it later.

The problem I think is that the code uses a jQuery click() method which does not work with keyboard events. Only .click() works with keyboard events.

> role=combobox/option The difference between combobox and menu: * combobox has a text field and a option list. If the option is changed, the text field is also updated. * menu has a button and a menu list, the menu item only triggers actions, doesn't change the menu button. So, in my mind that's the proper roles in my mind, because the dropdown behaves like a combobox (but in some cases it is used as a menu).

I guess. I see you also have to code to set it to listbox but that doesn't tend to work? For instance, the language list at the bottom of the page is marked combobo, so are the two menus at the top right.

[Jookia]

I looked at the collaboration fix you did. I initially thought it didn't work because I couldn't rebuild it, but I find the new solution unacceptable: You don't have to press enter to confirm your selection. If you go through the box list and leave the box it changes it anyway.

[wxiaoguang]

I looked at the collaboration fix you did. I initially thought it didn't work because I couldn't rebuild it, but I find the new solution unacceptable: You don't have to press enter to confirm your selection. If you go through the box list and leave the box it changes it anyway.

Then let's use action instead of onChange. Now, only the Click and Enter will send the AJAX request.

What's happening is that once the menu opens, the next tab is to the first menu item instead of the next menu. Once you tab the menu closes and you're lost.

It's still a little diffcult for me to reproduce. Could you provide detailed steps (using a real page UI)? For example, open page A, focus element X, press Tab, (see what) press Tab, (see what), etc.

[Jookia]

The clicking now works, but the button doesn't change to show the result now? So if I change it to 'read' then it will update only if I refresh. It remembers what I pressed in the menu though.

Guide for reproducing: Open http://localhost:3000/adminadmin/test/settings/collaboration, focus address bar, starting tabbing until you open the create menu. Now tab again. The second menu doesn't come up. You're instead focused on the first menu's first item.

[wxiaoguang]

The clicking now works, but the button doesn't change to show the result now? So if I change it to 'read' then it will update only if I refresh. It remembers what I pressed in the menu though.

I think it behaves well in most cases now.

Guide for reproducing: Open http://localhost:3000/adminadmin/test/settings/collaboration, focus address bar, starting tabbing until you open the create menu. Now tab again. The second menu doesn't come up. You're instead focused on the first menu's first item.

Oh, that's helpful, improved and added more comments.

[wxiaoguang]

About: role=combobox/option
I guess. I see you also have to code to set it to listbox but that doesn't tend to work? For instance, the language list at the bottom of the page is marked combobo, so are the two menus at the top right.

That's what I mentioned (but in some cases it is used as a menu).. Such cases can be improved by introducing a dedicated CSS class (or setting the role directly) for them in future, instead of guessing the role. So I think if there is no other fundamental problems in this PR, we can merge it first and improve these menu elements in separate PRs.

[Jookia]

This all seems to work good. There are some issues where I've managed to focus and open the menus without Orca entering focus mode- but this might just be some Linux a11y bug. It seems to coincide with system lag. It might be something to do with me accidentally enabling caret browisng, not sure.

Edit: To be clear: I'm 100% for this being merged but I would like @Flameborn to check out this focus issue on MacOS.

[wxiaoguang]

After some testing, it seems that menu/menuitem behaves better than combobox/option. So I just made another commit, please have a try.

[Jookia]

Looks and sounds good to me.

I did just find that setting the language doesn't work, but this might be a click() thing again.

[wxiaoguang]

Hmmm, I think I know the click() problem completely now. Let me see how to fix it without side-effects.

---

update: Fixed, now all <a> menu items can be clicked by Enter, including New Repo / Profile / Language.

And added more comments:

  // expected user interactions for dropdown with aria support:
  // * user can use Tab to focus in the dropdown, then the dropdown menu (list) will be shown
  // * user presses Tab on the focused dropdown to move focus to next sibling focusable element (but not the menu item)
  // * user can use arrow key Up/Down to navigate between menu items
  // * when user presses Enter:
  //    - if the menu item is clickable (eg: <a>), then trigger the click event
  //    - otherwise, the dropdown control (low-level code) handles the Enter event, hides the dropdown menu

[wxiaoguang]

This PR is ready for review.

[Jookia]

I did a quick test and it seems to work fine.

[silverwind]

@wxiaoguang make fomantic now produces a diff in web_src/fomantic/build/semantic.js which restores the parts you removed, which means it blocks us from rebuilding fomantic for further customization. Can you please find a solution that makes the removal apply during make fomantic? Maybe with a .patch file?

[wxiaoguang]

I have run make fomantic when submitting this PR, the dropdown code has been reverted to the official.

I tried make fomantic again on main branch, the only difference is EOL, no code change. (I do not know how the EOL difference comes .... but it doesn't affect code)

Maybe the EOL could be improved in another PR when upgrading Fomantic UI.

[silverwind]

It's not EOL-only for me. This is my result after make fomantic:

silverwind@8beb3ad

Not sure why you can't reproduce.

[wxiaoguang]

But the web_src/js/vendor/dropdown.js has been deleted, and the cp -f web_src/js/vendor/dropdown.js $(FOMANTIC_WORK_DIR)/node_modules/fomantic-ui/src/definitions/modules has been removed. Where does the code come from?

Can you reproduce it on a clear clone?

Or can you do a grep to find where is the string This version has been modified by Gitea to improve accessibility. ?

[silverwind]

Hmm yes, it doesn not reproduce on fresh checkout, must be related to untracked files. EOL diff should still be fixed though.

Search result does show some matches:

$ grep -r 'This version has been modified' *
web_src/fomantic/node_modules/fomantic-ui/src/definitions/modules/dropdown.js: * This version has been modified by Gitea to improve accessibility.
web_src/fomantic/build/components/dropdown.js: * This version has been modified by Gitea to improve accessibility.

[wxiaoguang]

web_src/fomantic/node_modules/fomantic-ui/src/definitions/modules/dropdown.js

Maybe you have copied the modified file to there manually, then your every build will use it. Changing files under node_modules seems not a good idea 😂

[silverwind]

Might be because previous patches were actually done inside node_modules and it is was never completely refreshed.

Regarding CRLF, we need to normalize. Normally, I'd use tools like dos2unix but I'm not sure whether this is universally available.

[silverwind]

#19932 will fix it.