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

Enhancement: improved the accessibility of codeblocks #3178

Closed
wants to merge 6 commits into from

Conversation

McZenith
Copy link

solves #3119

As highlighted at #3119 Changing code block from initial span to code HTML tag improves accessibility for screen readers most especially.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 31, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jul 31, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 03c47eb

https://deploy-preview-3178--docusaurus-2.netlify.app

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Unfortunately, this change seems to totally break Docusaurus 2 website code blocks.

https://deploy-preview-3178--docusaurus-2.netlify.app/build/docs/next/markdown-features#code-title

image

@McZenith
Copy link
Author

McZenith commented Jul 31, 2020

Wow! I'm going to look into this thanks!

@McZenith
Copy link
Author

McZenith commented Aug 1, 2020

It has been resolved! Since the goal is to ensure screen readers do not read codeblocks I have added aria-hidden="true" attribute to the span tag and It worked!

@slorber
Copy link
Collaborator

slorber commented Aug 1, 2020 via email

@McZenith
Copy link
Author

McZenith commented Aug 1, 2020

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-hidden_attribute

I feel removing codeblocks from accessibility tree could help solve the problem as written in the guide above.

I'm also relatively new to accessibility API but if you have a better way to go about it, I'd be glad to implement it.

Thank you.

@slorber
Copy link
Collaborator

slorber commented Aug 3, 2020

I'm not either an accessibility expert.
Unfortunately not sure I want to merge such PR if we are unable to find an authoritative source that says it's a good practice.

If you know a dev website that care a lot about accessibility, maybe we can check how they handle the code blocks and ask them for feedback.

@slorber
Copy link
Collaborator

slorber commented Aug 3, 2020

@McZenith it looks to me we should just use

{code}
, as it's something I see in many places including sites like W3 (https://www.w3.org/WAI/tutorials/forms/instructions/) etc...

@@ -21,9 +21,13 @@ export default ({children, className}) => {
{({style, tokens, getLineProps, getTokenProps}) => (
<pre className={className} style={{...style, padding: '20px'}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we have pre, but we should add code too I think

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @slorber, I will look into it and make necessary changes... I really appreciate your help and I just learnt something new...

@slorber slorber marked this pull request as draft August 3, 2020 14:09
@McZenith
Copy link
Author

McZenith commented Aug 4, 2020

@slorber Thanks for your guide this far. I really enjoyed the ride... I know it's not easy dealing with a newcomer but you helped me out and here it is, done... Thank you...

@McZenith McZenith changed the title Changed codeblock to <code /> in Docusaurus theme for accessibiity purpose Enhancement: improved the accessibility of codeblocks Aug 4, 2020
@slorber
Copy link
Collaborator

slorber commented Aug 5, 2020

np, happy to help you :)

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Unfortunately I can't merge this yet.

You should use real CSS, not inline styles.

+, this is an accessibility only PR, and the design should not be modified.

Before:

image

After:

image

I'm only willing to merge this PR if you make it so that the design is exactly the same as it was before

padding: '0px',
whiteSpace: 'pre-wrap',
wordBreak: 'break-word',
}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use the css module styles instead of inline styles

Copy link
Author

Choose a reason for hiding this comment

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

Oh, okay! Copy that! Next commit in a jiffy. Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

Done!!!

@McZenith McZenith marked this pull request as ready for review August 7, 2020 09:12
@McZenith McZenith requested a review from slorber August 7, 2020 14:01
@NikitaIT
Copy link

NikitaIT commented Aug 7, 2020

I looked at the code, it doesn't look bad.
But as far as I know, any changes should be justified in tests. From the solutions that I saw: a11y (for the storybook) and axe, but if this is not in the project, then this issue should be considered separately. Unfortunately, I do not have sufficient expertise in writing accessibility tests, neither with the provided libraries nor without them.
I think it would be possible to bring in someone who is really well versed in this matter. If there is no such person, then the current solution is obviously better than the previous one.

}}>
<code style={{...style, padding: '20px', display: 'inline-flex'}}>
{tokens.map((line, i) => (
<div
Copy link

@NikitaIT NikitaIT Aug 7, 2020

Choose a reason for hiding this comment

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

The only thing I don't like is that in the code div tag. It doesn't seem to be valid. Try it on:
https://validator.w3.org/nu/#textarea

<!DOCTYPE html >
<html lang="en">
<head><title>d</title></head>
<body>
<pre>
   <code>
     <div><span>werwer</span> </div>
      <div><span>werwer</span> </div>
    </code>
</pre>	
</body>
</html>

Output:

Error: Element div not allowed as child of element code in this context. (Suppressing further errors from this subtree.)
From line 9, column 15; to line 9, column 19
          <div><span>
Contexts in which element div may be used:
Where flow content is expected.
As a child of a dl element.
Content model for element code:
Phrasing content.

Phrasing content: https://html.spec.whatwg.org/multipage/dom.html#phrasing-content-2

Copy link
Author

Choose a reason for hiding this comment

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

Ohw okay, I will look into it and see what I can do. I initially didn't include it but I noticed the code highlights were not functioning properly. So I added it and it worked as expected.

@NikitaIT
Copy link

NikitaIT commented Aug 7, 2020

Also, I'm not sure if the text is clearly visible here. This does not apply to this pr. But it's worth checking out as accessibility issue.
https://deploy-preview-3178--docusaurus-2.netlify.app/build/blog/2018/04/30/How-I-Converted-Profilo-To-Docusaurus
image

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

bootstrap theme has inline styles

whiteSpace: 'pre-wrap',
wordBreak: 'break-word',
}}>
<code style={{...style, padding: '20px', display: 'inline-flex'}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

still has inline styles here

Copy link
Author

Choose a reason for hiding this comment

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

done! All solved!

@McZenith McZenith requested a review from slorber August 15, 2020 02:17
</div>
<pre className={styles.preTag}>
<code className={styles.codeTag}>
<div key={i} {...lineProps}>

Choose a reason for hiding this comment

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

this is still not valid html

Choose a reason for hiding this comment

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

And I think a comment is needed on why exactly pre>code>span is done here in the code, in both places, since this is not covered by tests, this is the only way to guarantee validity in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks, I'm not an expert but why is this not valid?

Choose a reason for hiding this comment

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

Copy link

@NikitaIT NikitaIT Aug 19, 2020

Choose a reason for hiding this comment

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

@slorber I would still recommend setting up tests to check accessibility and validity. Any mistakes can be critical for those who care.
In addition, it is often useful for correct indexing, the search on the site is of course important, but when the developer does not know something, he usually uses google to search, and internal search is the second step.
It may be worth asking the community for help in this matter. And to do it so that it is really seen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how can we setup accessibility and validity tests? how do we ask for such help to the community for you?

I don't know much about these topics but any help is definitively welcome

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks, but still not good 🤪

These 2 pages are still different:

https://deploy-preview-3178--docusaurus-2.netlify.app/build/docs/next/markdown-features
https://v2.docusaurus.io/docs/next/markdown-features

This PR is about accessibility, not design, so the expected result is to have improved accessibility, yet keeping the EXACT same design. Here the new design is different, and looks worst to me.

@slorber
Copy link
Collaborator

slorber commented Oct 19, 2020

Closing due to inactivity

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

Successfully merging this pull request may close these issues.

5 participants