Skip to content

Improve accessibility for issue comments #22612

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

Merged
merged 6 commits into from
Feb 23, 2023
Merged

Improve accessibility for issue comments #22612

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
470633c
[Accessibility] Fix markup for issue comments
Menelion Jan 26, 2023
4561586
Merge branch 'main' into fix-comment-heading
lunny Jan 29, 2023
4d0ee90
Merge branch 'main' into fix-comment-heading
lunny Feb 2, 2023
54de3fa
Merge branch 'main' into fix-comment-heading
jolheiser Feb 2, 2023
08c69d5
Change reactions div role to note
Menelion Feb 22, 2023
391da9d
Merge branch 'main' into fix-comment-heading
Menelion Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions templates/repo/issue/view_content.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
</a>
{{end}}
<div class="content comment-container">
<div class="ui top attached header comment-header df ac sb">
<div class="ui top attached header comment-header df ac sb" role="heading" aria-level="3">
Copy link
Member

@delvh delvh Jan 27, 2023

Choose a reason for hiding this comment

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

Hmm, is that really a heading?
I mean, it doesn't have any title-like attributes, and doesn't behave exactly like a title…

Copy link
Contributor

@tallship tallship Jan 27, 2023

Choose a reason for hiding this comment

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

Isn't the objective just to use Aria article like accessibility for readers?

Copy link
Contributor

@fsologureng fsologureng Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, is that really a heading? I mean, it doesn't have any title-like attributes, and doesn't behave exactly like a title…

The whole container functions like a comment head, but change the container to a H3 it has a deep impact in visual presentation. That's why this proposition does an aria fix.

Copy link
Contributor Author

@Menelion Menelion Jan 28, 2023

Choose a reason for hiding this comment

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

Exactly, @fsologureng is totally right. That's btw how it's done on Github: the whole username commented... thing is wrapped into a heading.
The article is good but not enough for navigation. Navigation by headings is in the blood of most blind web users, and a page that lacks headings is extremely inconvenient to navigate through. If you have an article with the comment, you have to: go to the article, then arrow up, i.e., back to see the author, then arrow down again past the author to read the comment text now. This is super inconvenient as it requires moving back and forth in the same text. Please note that blind users A) don't use mouse, only keyboard, and B) we navigate in a linear way, we always arrow up and down (we also use the Tab key, but it's out of this scope).

Copy link
Contributor

@fsologureng fsologureng Jan 29, 2023

Choose a reason for hiding this comment

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

How important it is that designers understand this.

<div class="comment-header-left df ac">
{{if .Issue.OriginalAuthor}}
<span class="text black bold">
Expand Down Expand Up @@ -69,7 +69,7 @@
{{end}}
</div>
</div>
<div class="ui attached segment comment-body">
<div class="ui attached segment comment-body" role="article">
<div class="render-content markup" {{if or $.Permission.IsAdmin $.HasIssuesOrPullsWritePermission $.IsIssuePoster}}data-can-edit="true"{{end}}>
{{if .Issue.RenderedContent}}
{{.Issue.RenderedContent|Str2html}}
Expand All @@ -85,7 +85,7 @@
</div>
{{$reactions := .Issue.Reactions.GroupByType}}
{{if $reactions}}
<div class="ui attached segment reactions">
<div class="ui attached segment reactions" role="complementary">
Copy link
Member

@delvh delvh Jan 27, 2023

Choose a reason for hiding this comment

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

complementary for reactions?
This doesn't seem right, if I read the MDN docs correctly…

Copy link
Contributor

@zeripath zeripath Jan 27, 2023

Choose a reason for hiding this comment

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

probably should be comment AFAICS

Copy link
Contributor

@fsologureng fsologureng Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that is not good for this. For landmark roles «it is not as good practice to use non-semantic elements, such as <div>, adding semantics with landmark roles».
Instead, I would suggest a menu role.

Copy link
Contributor

@fsologureng fsologureng Jan 27, 2023

Choose a reason for hiding this comment

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

probably should be comment AFAICS

comment role should be for the whole comment container (line 29)

Copy link
Contributor Author

@Menelion Menelion Jan 28, 2023

Choose a reason for hiding this comment

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

@fsologureng Gitea is already overusing the menu role, it's full of unlabeled menus. Actually what I would do is to combine the reactions with the comment itself, so it would be in the same article, but I'm not confident enough that if I wrap the whole thing in one more div, it won't upset the visuals.
First I worried that I didn't know about the comment role, but then I saw why. It's actually in a draft state for ARIA 1.3, meaning that probably no screen reader supports it yet. And yes, even when it gets out of draft, it would be nice to mark the whole comment up with it.
I agree that complementary is not the best fit for reactions. What about note then?

Copy link
Member

@delvh delvh Jan 28, 2023

Choose a reason for hiding this comment

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

A note role suggests a section whose content is parenthetic or ancillary to the main content.

Yep, sounds fine.

Copy link
Contributor

@fsologureng fsologureng Jan 29, 2023

Choose a reason for hiding this comment

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

@fsologureng Gitea is already overusing the menu role, it's full of unlabeled menus. Actually what I would do is to combine the reactions with the comment itself, so it would be in the same article, but I'm not confident enough that if I wrap the whole thing in one more div, it won't upset the visuals. First I worried that I didn't know about the comment role, but then I saw why. It's actually in a draft state for ARIA 1.3, meaning that probably no screen reader supports it yet. And yes, even when it gets out of draft, it would be nice to mark the whole comment up with it. I agree that complementary is not the best fit for reactions. What about note then?

Yes @Menelion, that bad design of menus should change. I have a PR repairing the label issue of reactions, but is not ready yet. I think both changes could be complementary.

Understood your apprehension about comment, I didn't aware of that.

note is for content and for complement main resource content. Reactions, instead, are essentially a set of actionable things. What about group, which is more generic but allows a differentiation with the rest of the comment body. I think it should works.

{{template "repo/issue/view_content/reactions" Dict "ctx" $ "ActionURL" (Printf "%s/issues/%d/reactions" $.RepoLink .Issue.Index) "Reactions" $reactions}}
</div>
{{end}}
Expand Down
6 changes: 3 additions & 3 deletions templates/repo/issue/view_content/comments.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
</a>
{{end}}
<div class="content comment-container">
<div class="ui top attached header comment-header df ac sb">
<div class="ui top attached header comment-header df ac sb" role="heading" aria-level="3">
<div class="comment-header-left df ac">
{{if .OriginalAuthor}}
<span class="text black bold mr-2">
Expand Down Expand Up @@ -69,7 +69,7 @@
{{end}}
</div>
</div>
<div class="ui attached segment comment-body">
<div class="ui attached segment comment-body" role="article">
<div class="render-content markup" {{if or $.Permission.IsAdmin $.HasIssuesOrPullsWritePermission (and $.IsSigned (eq $.SignedUserID .PosterID))}}data-can-edit="true"{{end}}>
{{if .RenderedContent}}
{{.RenderedContent|Str2html}}
Expand All @@ -85,7 +85,7 @@
</div>
{{$reactions := .Reactions.GroupByType}}
{{if $reactions}}
<div class="ui attached segment reactions">
<div class="ui attached segment reactions" role="complementary">
{{template "repo/issue/view_content/reactions" Dict "ctx" $ "ActionURL" (Printf "%s/comments/%d/reactions" $.RepoLink .ID) "Reactions" $reactions}}
</div>
{{end}}
Expand Down