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

Improve milestone title #22853

Closed
wants to merge 1 commit into from
Closed

Improve milestone title #22853

wants to merge 1 commit into from

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Feb 10, 2023

Pulled from:

font-size: 16px;
min-width: 0;
font-weight: 600;

Before:
image

After:
image

- Don't use `h2`(which is also bad for a11y reasons) for the title, instead use `font-size` and
`font-weight` to make the title look better.
- Forgejo issue: https://codeberg.org/forgejo/forgejo/issues/348
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 10, 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 Feb 10, 2023
@yardenshoham yardenshoham added the topic/ui Change the appearance of the Gitea UI label Feb 10, 2023
@fsologureng
Copy link
Contributor

Removing item headings prevents screen readers from navigating easily. Improving size should not go against semantics. DIV is one of the least meaningful elements in an HTML structure.

The H1 should wrap the Labels|Milestones button, as in Releases (which should have an H1 and not an H2).

@fsologureng
Copy link
Contributor

If you still want to change to div, could add role="heading" aria-level="2" to preserve aria behavior.

@lafriks
Copy link
Member

lafriks commented Feb 10, 2023

I don't think they can really be defined as headers. Probably role listitem would be more appropriate

@fsologureng
Copy link
Contributor

I don't think they can really be defined as headers. Probably role listitem would be more appropriate

That's for the <li> which has the listitem role by default. I mean the content inside each list item, which have a title and a content. It's not very well structured but it's a unit. For that kind of content, a screen reader sequentially goes through the list reading headings until the user hears which one he wants to select to inspect its full content.
Navigation is through landmarks and by levels, and the headings serve as elements of an index.

@delvh delvh self-requested a review February 10, 2023 20:53
@Gusted
Copy link
Contributor Author

Gusted commented Feb 11, 2023

This is no different than the issue list. If there's a good a11y solution, it also should be applied to the lists (in another PR). The milestone one is currently standing out at the moment with its <h2> (for no good documented reason).

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 11, 2023

You can just use a h3 tag (or even h4), then no need to add more CSS styles.

The pre-defined styles are good enough and more general.

Screenshot: the first one is h3, the second one is h2

image

@Gusted
Copy link
Contributor Author

Gusted commented Feb 11, 2023

You can just use a h3 tag (or even h4), then no need to add more CSS styles.

That's misusing the heading elements(which currently is already happening), they should follow in logical order. MDN confirms this. Best solution is to figure out which aria attributes must be set and also apply them to the issue/pull list.

@fsologureng
Copy link
Contributor

You can just use a h3 tag (or even h4), then no need to add more CSS styles.

That's misusing the heading elements(which currently is already happening), they should follow in logical order. MDN confirms this. Best solution is to figure out which aria attributes must be set and also apply them to the issue/pull list.

Misused or not, they are useful right now and they are going to be removed. That's a degradation of the poor accessibility of this page, and for a solution that only improves the appearance.

If there is another lack of a11y (with a fix in progress) it doesn't mean that the criteria should bring everything towards that state, that would be levelling down.

Fixing the bad HTML structure with aria markup is the last solution to apply for accessibility, the first one (literaly) is to use the HTML elements made for it.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 11, 2023

I also agree that it's not worth to follow the standard concerns (MDN heading order, well, not the standard) too strictly. I care more about making the code clearer and easier to be maintained, and be functional enough for various situations, like a11y.

@Gusted
Copy link
Contributor Author

Gusted commented Feb 11, 2023

Closing.

@Gusted Gusted closed this Feb 11, 2023
@Gusted Gusted deleted the forgejo-348 branch February 11, 2023 14:59
@wxiaoguang
Copy link
Contributor

Thank you Gusted. Since this PR is closed, there is a new one #22863 , and the new one fixes one more milestone <h2> problem (on the user dashboard, the same problem)

According to the previous comments (#22853 (comment) , #22853 (comment) ), and confirmed with fsologureng , nothing is changed, so the new PR still uses the <h?> tag to keep the current a11y behavior, to prevent from uncertain changes. The a11y related problems could be fixed together in the future by new PRs.

@Gusted
Copy link
Contributor Author

Gusted commented Feb 11, 2023

Fine with me, issue is fixed. I don't follow what to know what to follow.

lunny added a commit that referenced this pull request Feb 12, 2023
Replace #22853 since it's closed, and actually there are 2 places need
to be fixed.

~~Follow @fsologureng 's suggestion to keep the `<hX>` tags.~~ 

Update: from fsologureng: this doesn't change anything from a11y's point
of view. So I think this PR could be fine to fix the UI looking problems
as a quick patch, then defer the a11y problems to new PRs together.

Before: the font-size is too large.

After: it seems better.

![image](https://user-images.githubusercontent.com/2114189/218266257-fc2d5872-9e96-4c6a-87ea-f27531ac15c0.png)

![image](https://user-images.githubusercontent.com/2114189/218266247-efc09d83-405f-4495-967a-30d9744134ce.png)

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants