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

A11y improvement #2322

Merged
merged 18 commits into from
Feb 4, 2024
Merged

A11y improvement #2322

merged 18 commits into from
Feb 4, 2024

Conversation

krvpb024
Copy link
Contributor

@krvpb024 krvpb024 commented Jan 27, 2024

Do you follow the guidelines?

Improvement

I have tested the screen reader on Android (TalkBack), iOS (VoiceOver), macOS (VoiceOver), Linux (Orca).

Add skip to content link

Skip links can help keyboard users navigate to the main content instead of tabbing through all the focusable elements.
This technique is recommended by the W3C

Add nav landmark to page-header links

Wrap the nav tag around page-header links so that the screen reader user can navigate to them through landmarks.
Add a label for the page header nav according to the WAI ARIA Authoring Practices Guide.
I didn't add a label for the first nav because the nav in the header already has a semantic meaning for global nav.

Search landmark

Add a search landmark for the search form so that the screen reader user can navigate to it through landmark.
I moved the search element out of nav because when the nav css display is none, screen reader can't discover there is a search landmark. It's difficult to find a search form for screen reader users when in mobile layout.

Improve a11y for disclosure menu

The current mobile layout will hide nav element, but there's no clue for the screen reader user to notice that tapping the miniflux logo will expand the nav menu. This also happened to the search form.
I added the aria attribute to the nav element and wrapped details and summary tag in the search form to fix this problem.

Search form

Using placeholder for labeling input would cause some problems, so I added aria-label to fix this problem.
And I added a submit button for mouse and trackpad users in order to perform search actions.

Article element

Remove the redundant article role.
Add a h2 heading to the article element so that the screen reader user can navigate the article through the heading level. This would also fix the issue that screen reader users encountered with the current keyboard navigation method.
Add an aria-label for the article element for the screen reader, and when they navigate by landmark, they will have a better understanding what this article is about.
Remove the icon image alt in feed_list, it's the same as the feed title. It didn't provide extra meaning for the screen reader users, but it will cause them to have to navigate twice in order to get the next element.

Add sr-only css class for screen reader user

sr-only class will hide the element from the page but can still be discovered by the screen reader.
For example:

Example (2)
The screen reader will say, "Example: Number of unread entries: 2"

This will help the screen reader user know what's the meaning of the number is.

Differentiate between buttons and links.

Buttons and links have different semantic meanings. Screen reader users can navigate these two elements separately. Changing links that could perform actions to buttons can improve the user experience.

Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

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

The single entry page layout looks slightly different. Is it possible to keep the styling like it was before?

Example with your changes:

image

Current page layout:

image

@sonor3000
Copy link

sonor3000 commented Feb 1, 2024 via email

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 1, 2024

@sonor3000 Thanks for your feedback. May I ask which screen reader and device you are using? I'll take a look at the jump to first item problem.

The article list position may be fixed by adding feed role and set aria-posinset, aria-setsize attributes.

I've thought about differentiate links and buttons. I didn't change that is because I thought it won't affect user experience that much, so thank you for pointing out that.

@fguillot I'll work on these issue above. Thanks for your review recently.

@sonor3000
Copy link

sonor3000 commented Feb 1, 2024 via email

@sonor3000
Copy link

sonor3000 commented Feb 1, 2024 via email

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 2, 2024

@sonor3000 I've tested the feed role and aria attribute solutions. Maybe because this role is a new feature, Android TalkBack, VoiceOver, Linux Orca didn't announce the position value.

But I found that when navigated to the next headings on the last article, TalkBack and VoiceOver will announce they can't find the next heading, and Orca will jump to the first item and announce this action. So I think this problem can be solved by adding heading role for article element.

@sonor3000
Copy link

sonor3000 commented Feb 2, 2024 via email

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 2, 2024

@sonor3000 I've noticed that when the confirmation interface pops up (e.g., clicking "Mark all as read" will show "Are you sure? Yes, no"), the screen reader will lose the current focused positions. Does this behavior affect you while you're using?

If we use the HTML dialog in modal mode, the browser will handle the inert and autofocus the dialog.

If you hit the tab key repeatedly, you’ll notice that only the content within the dialog can get focus while the modal dialog is open. Everything outside the modal dialog is inert as long as the dialog is open.
web.dev - Dialog

And also, get the ESC keyboard shortcut to close the dialog. But the only problem is that it will change the layout visually.

@fguillot Do you suggest that we change the current user interface into a dialog?

@sonor3000
Copy link

sonor3000 commented Feb 4, 2024 via email

@fguillot
Copy link
Member

fguillot commented Feb 4, 2024

@fguillot Do you suggest that we change the current user interface into a dialog?

I have never used the dialog element. I'm open to contributions if it's the right thing to do. It looks like it's supported by most browsers nowadays. However, it would be better to do that in another pull-request. There is already lot of changes in this one.

@fguillot fguillot merged commit 8367413 into miniflux:main Feb 4, 2024
15 checks passed
@krvpb024 krvpb024 deleted the a11y_improvement branch February 4, 2024 18:53
@sonor3000
Copy link

sonor3000 commented Feb 4, 2024 via email

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 5, 2024

@sonor3000 This PR might meet your request.
#2328 (comment)

@distantorigin
Copy link

I'm a screen reader user and I happen to see these changes because I have Watchtower set up to pull down the latest nightly docker images.

I use NVDA and JAWS on Windows, primarily working in Firefox but also Chrome.

These updates have entirely changed the way entries are read out, and, on Windows at least, not for the better. This has made navigating without the keyboard shortcuts easier, but for those that use these (as opposed to screen reader navigation), there are a few issues.

Navigating with j/k via the keyboard between entries, I now hear:
Hacker News: Front Page Meta cuts off third-party access to Facebook Groups article
Hacker News: Front Page Meta cuts off third-party access to Facebook Groups heading
Hacker News: Front Page Meta cuts off third-party access to Facebook Groups link

All for a single item. This is likely due to the alt text on the feed icons, though I would strongly reconsider having heading roles for these items as it is not semantically correct, even though it may make certain flows more usable based on how each user uses their screen reader.

Additionally, the feed name is now read out before the actual article, which was not behavior exhibited before.

Categories are also read differently (this is getting added to the aria-label, it seems):
Technology, page.categories.unread_counter: 1150

Instead of "Technology (1150)". The same behavior as entries is also exhibited when using j/k to navigate; items are read three times.

Mark all as read buttons for each category use an aria-describedby attribute that points at the category title, which results in the category being read out again when this button is focused.

I'm sure there are others that I haven't caught yet. Screen reader users are not a monolith, and everyone has different models of interacting with content. I propose staying with the original design, which was more semantically correct and allowed users to navigate by the article landmarks if they wish to add this to the navigation roter on mobile or bind a key using their screen reader of choice. This model has worked well for years, and although I recognize and applaud you for implementing accessibility, this changes the paradigm significantly and is overly verbose for those that may not need explanations of where they are.

I'm open to discussion and comments on this; I stumbled upon this issue trying to track down the commits that were behind these changes, and I'm glad to see some ongoing conversation.

@distantorigin
Copy link

Additional a11y bug: making options buttons is semantically incorrect. For example, we now have buttons by feeds that read:

If this is a button, it should use a <button> element. Using the button role for screen reader convenience violates WCAG guidelines for some, but not all of these cases. "External Link" opens another page, which is the function of a link. "Read" and "Star" make sense as buttons if they appear visually as buttons as well. Not to mention, "Button external link" is surely a bit contrary. External Link aside, this applies for all other entry options, and the aria-describedby causes the article title to be read along with every single button.

@fguillot Please consider reverting these changes until we can reach a consensus on what this should look like. If this gets rolled into a final release it will be confusing for screen reader users at best (specifically those that are already used to the layout prior to this set of changes), and downright problematic at worst.

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 6, 2024

@distantorigin

......though I would strongly reconsider having heading roles for these items as it is not semantically correct

Why do you consider it is not semantically correct?

......when using j/k to navigate; items are read three times.

Does items are read three times affect user experience a lot?

Categories are also read differently (this is getting added to the aria-label, it seems):
Technology, page.categories.unread_counter: 1150

After the translation improvement, this key is removed. Could you help on this one, @fguillot ?

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 6, 2024

I'm a screen reader user, use Android TalkBack most of the time, in desktop I use Linux Orca and macOS VoiceOver.

I made the changes base on the concept, no matter which way you use, should get enough information. According to WebAIM Screen Reader User Survey for Finding Information:

67.7% of screen reader user navigate through headings
7.1% of screen reader user navigate through links
3.2% of screen reader user navigate through landmarks

Navigate through the landmarks is the first choice when I'm using screen reader. Article is counted as landmark in Android TalkBack. But the problem I encountered is TalkBack won't announce the text when it's a link when navigate through landmark on article element. The way to fix this is to set the aria label to the article element. So people who preferred to navigate through landmarks can get enough information.

Add heading role can let people navigate through headings, and which is the method that has most user preferred to. And also my second choice when the page didn't set landmark correctly. For example, in category entries page, heading level one tell user that what the category is, and heading level two, the article title, indicate that is article is the subsections of this category.

Article landmark, headings, links happened to be the same in this case, but it won't have to be. For example, Article landmark can set the label to announce more information just than article title, it could add feed name, category, maybe possible read time or small part of the paragraph in aria-describedby. Heading should stick to article title is more semantic correct. And as for the link, its actual meaning is "go to the entry page", instead of direct wrap the whole heading text as link, set the actual meaning is more semantic correct.

I think we could edit the label for each role specifically as improvement. But I think these three roles are labeled by article title has announced the enough information to users. User may think it's redundant that it announces the same text three times. But each role label has it's meaning when navigate by a different role.


The feed name is announced before article title, is because the layout display this way visually, the feed icon has alt text of the feed name is the first item in entry section. The title link is the second element of the entry section. But the keyboard navigation is focus on the title link, the feed icon is omitted. Img tag with alt text set means it has meaning to users, and actually it is.
When I'm not using screen reader, the title is more like the combination of the feed icon and the article title visually. Because it might have different feed that have the similar or the same title of the entry in category page. When I scan through the entries, the article source (feed icon) and article title are important for me to determine whether to read the content or not. So I expect when I'm using a screen reader I should get the same information as when I directly read on screen.


And for the aria-describedby for button, when I navigate through button, there are tons of button has the same label as read, star. Sometimes I'll get confused which entry is this button's target. But yeah, this could waste users time when use screen reader read all content function.

@distantorigin
Copy link

Why do you consider it is not semantically correct?

Because they aren't headings visually, nor are they sections of the page. These are, for all intents and purposes, posts/entries in a list. I would consider a feed role with the enclosed items as articles to be more valid, though I'm open to alternatives on this.

Does items are read three times affect user experience a lot?

Absolutely.

But the problem I encountered is TalkBack won't announce the text when it's a link when navigate through landmark on article element. The way to fix this is to set the aria label to the article element. So people who preferred to navigate through landmarks can get enough information.

I don't think it's our responsibility to accommodate for user agent deficiencies or screen reader bugs (this seems like a bug, at least.) Especially if the fix ends up degrading the experience for other screen readers and platforms.

Add heading role can let people navigate through headings, and which is the method that has most user preferred to. And also my second choice when the page didn't set landmark correctly.

Feed entries and category items are already enclosed in an unordered list, which is navigable by all popular screen readers, in my experience. I'm not sure I see the benefit here.

I think we could edit the label for each role specifically as improvement. But I think these three roles are labeled by article title has announced the enough information to users. User may think it's redundant that it announces the same text three times. But each role label has it's meaning when navigate by a different role.

It is redundant, and previously these were just announced as articles and links simultaneously with one copy of the text using NVDA. I'll need to do further testing with macOS. Unfortunately I don't have an Android device on hand. Having the entire item read once for each role that it inherits is bad UX, in my opinion. But I'm just one person.

When I scan through the entries, the article source (feed icon) and article title are important for me to determine whether to read the content or not. So I expect when I'm using a screen reader I should get the same information as when I directly read on screen.

I specifically objected to this for the following reasons:

  • It becomes very cumbersome to hear the same source repeatedly before the feed title if you're scrolling through many feed items, especially because some RSS feeds can publish many articles at one time.
  • If you're on a feed's page, the feed title is still read first. So if I'm on my individualized feed page for HackerNews, I'll hear "HackerNews: Front Page" before every single item. I know I'm on the HN feed, this is unhelpful and superfluous information to be providing.
  • It would make more sense to have what you described when using left/right arrow or J/K:
    • For example, Article landmark can set the label to announce more information just than article title, it could add feed name, category, maybe possible read time or small part of the paragraph in aria-describedby.
    • Probably in the order of article title, feed name (category) [if you're not on the feed's page], read time, preview

More than anything, this is showing me that there is--as I mentioned prior--not only one way of interacting. :) I just hope that we can keep all ways congruent with each other, as they were previously.

Maybe the overall answer is having an accessibility setting that allows you to configure if headings should be used for feeds and feed items, in what order items are announced when using the keyboard, etc. As this is a web app, I've always considered j/k and the arrows the most accessible and uniform way of navigating, since its also the way a non-screen reader user that uses the keyboard would navigate. But I am not the end all be all on interface usability.

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 6, 2024

@distantorigin
The heading is for describing the structure of this web page.

......headings with a lower rank start new subsections that are part of the higher ranked section.

So the category name as heading level one, informs that the article title as heading level two is related to this category.
The article HTML element represents a self-contained composition in a document. And article element can have heading elements inside. So it's totally valid usage.

If it doesn't look like a heading, it's more like a visual design problem.

So, the benefit for headings in article element is:

  1. have basic outline for understanding the page structure
  2. screen reader user can navigate entries through headings

Feed entries and category items are already enclosed in an unordered list, which is navigable by all popular screen readers, in my experience. I'm not sure I see the benefit here.

If the user is already familiar with this page, then the user might know there would be a list can be navigated through, what about the new user?
And people have different models of interacting with content, as I mentioned above WebAIM Screen Reader User Survey, When trying to find information on a lengthy web page, which of the following are you most likely to do first? 67.7% of people in this survey will try to navigate through the headings.
How is that not provide any benefit?

@distantorigin
Copy link

distantorigin commented Feb 6, 2024

@krvpb024 If we want to use headings as an alternative way to navigate, that's fine. I'll back down on this one, but we need to make sure all models of interacting, especially the existing models via keyboard only, are just as accessible. Currently these fixes cause regressions that I've detailed above. I will also posit: Having headings for each section of the page allows a new user to familiarize themselves very quickly. In version 2.0.51, you can navigate by heading to get to the Unread feeds (or categories, or feeds, depending on the page) and then skim further to see your feeds. Users do not singularly navigate by headings; it's a methodology of identifying what major elements exist on a page.

If it doesn't look like a heading, it's more like a visual design problem.

So you're saying the visual design should follow the accessibility design, even though it may not make sense to be that way? I'm not entirely sure what this means, can you clarify

At the very least, these changes should be togglable or configurable, much like themes are. Ideally, however, we can reach a solution that makes everyone happy. I will take a stab at the templates in use and see if I can propose something that retains the old functionality as well while carrying these changes forward for the headings approach.

Until we do reach a solution that does this, I hope that we can put these changes on hold as to reach a resolution that won't involve regressions or usability complications.

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 6, 2024

@distantorigin

It is redundant, and previously these were just announced as articles and links simultaneously with one copy of the text using NVDA.

No, it's not redundant. It doesn't announce that much information in previous version, is because previous version didn't provide such information for user who need that.
Navigate by landmark (Android TalkBack)/article (iOS VoiceOver) doesn't have a consist action. TalkBack will announce there is image, link, list inside, so you won't know the entry title. iOS VoiceOver will focus on the first element which is the img has the feed name set as alt text, so you always have to swipe once to next to know the entry title. So this is the reason why the aria-labelledby on article element has to set. To have a consist and better user experience.
And actually it didn't change the behavior to repeat the same text. It because in previous version the article element didn't have any label that screen reader should announce, so it just simply announce "article". The text that be thought as redundant is the information that other user's need when they use their preferred way to interact with content.
And it's not a bad UX design, different roles can have different label for semantic meanings. Just because they're the same in this case, doesn't mean they will always be the same. So it's complete understandable why screen reader have to announce all the roles and label.

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 6, 2024

@distantorigin I talked about its visual design problem is because when I ask you why do you consider it is not semantically correct? You mentioned:

Because they aren't headings visually, ......

@distantorigin
Copy link

@krvpb024 Yes, it did change the behavior. I'm talking about navigating via the keyboard with J/K or the arrows, not by landmark with your screen reader. Previously, this read the title of the article. Now, it reads three times when navigating with Miniflux's native shortcuts, once for each role. I've been able to replicate this with JAWS and NVDA.

Hacker News: Front Page Meta cuts off third-party access to Facebook Groups heading
Hacker News: Front Page Meta cuts off third-party access to Facebook Groups link

This is not ideal, and is very much redundant.

@fedgrant
Copy link

fedgrant commented Feb 6, 2024

Hey, previously the search bar was display:none and this changed removed that. Home come? Can we add that line back?

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 7, 2024

@fedgrant Hi, the search bar is wrapped by HTML details and summary element to control the display toggle. So the CSS display none is removed.
May I ask the screen reader you're using and how you interact with the display property when you're using?

@sonor3000
Copy link

sonor3000 commented Feb 7, 2024 via email

@distantorigin
Copy link

Agreed with everything you've noted here, @sonor3000. I don't have a Linux or Android device to test on currently, but your macOS experiences align with mine.

This is OK for me with Orca on Linux, no matter if I use h or j/k to
navigate through the cathegories. But on a Mac with VoiceOver the cathegory
is spoken twice.

This was fixed in commit 5ce5c47 and was a translations issue.

@sonor3000
Copy link

sonor3000 commented Feb 7, 2024 via email

@fedgrant
Copy link

fedgrant commented Feb 9, 2024

@krvpb024 I dont have a screen reader. I just noticed that the search bar appeared at the top of page and I had never seen it before and tracked it down to this change. This is what it currently looks like.

File

I liked it better without the extra search tab. Is there any way to revert that portion back?

@sonor3000
Copy link

sonor3000 commented Feb 9, 2024 via email

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 9, 2024

@fedgrant This change is because in the previous version, the search form was inside the nav element. And it was hidden by display: none; by default in the mobile layout. Screen readers won't announce the element if the value of display in CSS is none. Not hiding it by default can make search landmark more discoverable to screen reader users.

Doesn't this layout change affect your usage a lot?

@krvpb024
Copy link
Contributor Author

krvpb024 commented Feb 9, 2024

@sonor3000 I've made some changes to deal with this screen reader behavior in this PR #2348. I've tested on macOS VoiceOver and Windows NVDA, and it will only announce the title once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants