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

Add missing lang attribute #2538

Closed
wants to merge 1 commit into from
Closed

Conversation

Primigenus
Copy link
Contributor

Issue: #2351

What I did

Accessibility tests warn that the HTML element requires a lang attribute. See: https://dequeuniversity.com/rules/axe/2.4/html-has-lang?application=AxeChrome

Ideally the lang attribute's value would be set to whatever language the user is using for Storybook, but that would require adding configuration, so this seems like a short term fix that addresses the immediate issue.

How to test

Run accessibility tests on the iframe and find that they no longer warn about the HTML lang attribute.

Accessibility tests warn that the HTML element requires a lang attribute. See: https://dequeuniversity.com/rules/axe/2.4/html-has-lang?application=AxeChrome

Ideally the lang attribute's value would be set to whatever language the user is using for Storybook, but that would require adding configuration, so this seems like a short term fix that addresses the immediate issue.
@codecov
Copy link

codecov bot commented Dec 22, 2017

Codecov Report

Merging #2538 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2538   +/-   ##
=======================================
  Coverage   16.87%   16.87%           
=======================================
  Files         307      307           
  Lines        7792     7792           
  Branches      794      778   -16     
=======================================
  Hits         1315     1315           
- Misses       5825     5857   +32     
+ Partials      652      620   -32
Impacted Files Coverage Δ
app/react/src/server/iframe.html.js 73.91% <ø> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/babel_config.js 0% <0%> (-77.42%) ⬇️
...mments/src/manager/components/CommentList/index.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.63% <0%> (ø) ⬆️
...ponents/left_panel/stories_tree/tree_decorators.js 33.33% <0%> (ø) ⬆️
addons/knobs/src/components/types/Select.js 7.93% <0%> (ø) ⬆️
...ents/src/manager/containers/CommentsPanel/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
addons/a11y/src/components/Report/index.js 0% <0%> (ø) ⬆️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3ebc61...d7547a1. Read the comment docs.

@ndelangen
Copy link
Member

I get that this will solve some warning in some tool somewhere. I truly appreciate the effort of you fixing it.

I'm a little concerned about if by merging this we'd not actually be providing the user-agent with potential wrong information, and by that making it worse for some small percentage of users.

What do you think?

@Primigenus
Copy link
Contributor Author

Primigenus commented Dec 24, 2017

@ndelangen Yes, this is a problem. Per the website I linked:

When configuring a screen reader, users select a default language. If the language of a webpage is not specified, the screen reader will assume it is the default language set by the user. This becomes an issue for users who speak multiple languages and access website in more than one language. It is important to specify a language and ensure that it is valid so website text is pronounced correctly.

So if we leave it unset, the screen reader will use whatever the user has set as default.
If we set it to en then the screen reader will assume the contents of the Storybook frame are English, which may not be the case.

That's why ideally we allow the user of Storybook to set the language, which is then set in the lang attribute. It would be nice to amend this commit with that change, but it's more impactful than just setting one value now.

@danielduan
Copy link
Member

Since the iframe would display mostly story content, I would think that the viewer's screen reader is most likely also set to the story content as a default.

The only English we provide in the frame is from the info addon. I don't think that justifies assuming the entire page is en. If you really need screen reader compliance for Storybook, you can also add the lang attribute tag through javascript either in the stories or in preview-head.html.

@Primigenus
Copy link
Contributor Author

@danielduan Agreed that it doesn't justify assuming it for everyone. That's why I proposed making it configurable in my PR comment. What would it take to accomplish that?

Adding it to preview-head.html works, but it places the responsibility for the page served by Storybook being accessible on the developer. I think that responsibility lies with Storybook.

@danielduan
Copy link
Member

@Primigenus It could be an option within the Options addon but I'm still not sold on this:

it places the responsibility for the page served by Storybook being accessible on the developer

To me, if a component has labels in German and another component in French, the writer of the individual stories should have the responsibility to configure the story itself to change the lang attribute. I'm not quite sure how a component story could be added without having permissions to preview-head.html or the story component file.

To have it as an option in the addon is a possibility, but I'm concerned about maintainability in the long term since this doesn't appear to be a commonly requested feature.

Maybe others can chime in?

@stale
Copy link

stale bot commented Feb 16, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

@stale stale bot added the inactive label Feb 16, 2018
@Hypnosphi
Copy link
Member

Hypnosphi commented Feb 17, 2018

it places the responsibility for the page served by Storybook being accessible on the developer

Well, what's shown in preview iframe is the responsibility of the developer anyway.
We should definitely add this attribute on manager html (https://github.com/storybooks/storybook/blob/master/app/react/src/server/index.html.ejs, and similar for other frameworks), as our manager interface is in English

@Primigenus are you interested in adding that?

@stale stale bot removed the inactive label Feb 17, 2018
@danielduan
Copy link
Member

closing in favor of #3065

@danielduan danielduan closed this Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants