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

WIP Fixes accessibility behavior for topic management in the repository home view #22631

Closed

Conversation

fsologureng
Copy link
Contributor

These changes address the impossibility of selecting the anchor for click "Manage topics" in the repository home view and, once inside the management widget, the impossibility of selecting each current topic for the action of delete.
Contributed by @forgejo.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 27, 2023
@zeripath zeripath added topic/ui Change the appearance of the Gitea UI topic/ui-interaction Change the process how users use Gitea instead of the visual appearance labels Jan 27, 2023
@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 Jan 27, 2023
@zeripath zeripath added this to the 1.19.0 milestone Jan 27, 2023
Comment on lines 16 to +22
viewDiv.hide();
editDiv.css('display', ''); // show Semantic UI Grid
});
mgrBtn.on('keydown', (e) => {
if (e.key === 'Enter' || e.key === ' ') {
viewDiv.hide();
editDiv.css('display', ''); // show Semantic UI Grid
Copy link
Member

Choose a reason for hiding this comment

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

We could think about extracting the function body (without condition) into a separate method…

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use a button, then it can automatically handle enter as click.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is an example about how to use button correctly, instead of duplicating code again.

@@ -2331,6 +2331,7 @@ tag.create_success = Tag '%s' has been created.

topic.manage_topics = Manage Topics
topic.done = Done
topic.delete = Delete
Copy link
Member

Choose a reason for hiding this comment

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

Delete topic?
Or is it always clear for users what is supposed to be deleted?

Suggested change
topic.delete = Delete
topic.delete = Delete topic

@fsologureng
Copy link
Contributor Author

I have realised that this solution is unusable under the current behaviour of the related dropdown menu; inside the topic manager the delete topic buttons are not clickable and the newly added items contain a different structure so the solution will not work.
Also I've noticed some strange bugs doing back and forward with TAB while adding and deleting topics.

I will leave it as WIP until I understand the problems and develop a complete solution.

@fsologureng fsologureng changed the title Fixes accessibility behavior for topic management in the repository home view WIP Fixes accessibility behavior for topic management in the repository home view Jan 29, 2023
@fsologureng fsologureng marked this pull request as draft January 29, 2023 03:04
@fsologureng
Copy link
Contributor Author

fsologureng commented Feb 13, 2023

Hi @wxiaoguang: can you suggest me a way to change the accessibility pattern of this dropdown, which is not a menu but a combobox? The global modification of the fomantic static template for the dropdown menu prevents me to make that change, because the role of the items is not menuitem but option. I have tried to change this modification by a callback function through the Show event, but the fomantic bind function of the event is not stackable (I mean, it doesn't support two calls), so I will have problems with this kind of settings.

@wxiaoguang
Copy link
Contributor

I can only answers some questions based on my previous experiences. I am not reading the code at the moment, just my opinion, for information only.

a way to change the accessibility pattern of this dropdown?

If you can hijack the Fomantic UI's module, maybe you can change almost everything from it.

which is not a menu but a combobox?

According to my previous tests, https://github.com/go-gitea/gitea/blob/main/web_src/js/features/aria.md , the menuitem seems working better, while option has various problems when it's used with the dropdown - it's hard to describe, I just tested and felt at that time. Maybe one of the problems is that Fomantic UI dropdown could be used as a menu container, and it could also be used as an option container. You can try to find out whether there are better approaches, for example, introduce more fixes to use option instead?

The global modification of the fomantic static template for the dropdown menu prevents me to make that change ... but the fomantic bind function of the event is not stackable

Maybe you could read the Fomantic UI code to see whether there are other approaches and make more fixes to the dropdown, or replace the Fomantic UI module with other components.

@yardenshoham yardenshoham modified the milestones: 1.19.0, 1.20.0 Feb 22, 2023
@yardenshoham yardenshoham added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Feb 22, 2023
@wxiaoguang
Copy link
Contributor

I think this issue might answer some of your questions. The root problem is that the hard-coded focus management inside Fomantic Dropdown. I think it's unfixable at the moment.

[Proposal] Improve the dropdown step by step #23345

@techknowlogick techknowlogick added the topic/accessibility This issue/pull request wants to improve the accessibility label Mar 7, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 7, 2023

because the role of the items is not menuitem but option

If you think it causes problem, you could exclude it from the aria.js (make aria.js skip some unwanted elements). According to my previous tests, it might not work well because of the "focus" problem. I am not sure.


I proposed a new PR (the best I can do at the moment).

I think it could improve a11y a lot.

To resolve the screen reader problem fundamentally, IMO the component code should be rewritten ....


Update: since when discussing #23346 with fsologureng, there was some unclear (I don't understand) problems, I will discard it (my #23346). The totally new (from scratch) PRs are:

@wxiaoguang
Copy link
Contributor

Closed by #23542 and #23626 (due to the comment https://github.com/go-gitea/gitea/pull/22631/files#r1089583380 was nerve resolved and it doesn't handle dynamically generated labels)

@wxiaoguang wxiaoguang closed this May 2, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 2, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 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. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 topic/accessibility This issue/pull request wants to improve the accessibility topic/ui Change the appearance of the Gitea UI topic/ui-interaction Change the process how users use Gitea instead of the visual appearance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants