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 <html> tag and language #100

Merged
merged 1 commit into from
Jun 22, 2016

Conversation

jszwedko
Copy link
Contributor

WCAG 2.0, Guideline 3.1.1 requires that

"The default human language of each Web page can be programmatically
determined"

Discovered this while doing an accessibility audit of our Jenkins site and
tripped the
https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules#ax_html_01
rule of Google's automated accessibility tool.

@pathawks
Copy link
Member

There is exactly one word on the page.

What actual benefit is there to setting the language?

@benbalter
Copy link
Contributor

Is there any downside? Not to be too pedantic, but to avoid the obvious follow up issue, perhaps it should be {{ site.lang | default: "en-us" }}?

@jszwedko
Copy link
Contributor Author

I thought about that @benbalter, but since this page is strictly in English (even if the rest of the author's site is not), it felt like en-US was the most appropriate. I'm curious if you have more thoughts on this though.

@pathawks You can see some of the benefits of setting the default language for users using assistive technologies on https://www.w3.org/TR/UNDERSTANDING-WCAG20/meaning-doc-lang-id.html (under Specific Benefits of Success Criterion 3.1.1). Typically the user would never see this page as it should immediately redirect, but the benefit is for users where the page doesn't do this automatically (if they have JS disabled, e.g.). I agree that there isn't much text content on the page, but I don't see any obvious downsides to setting the language even if there was only 1 word (in this case there is 9 including the page title). I may be missing something though, please let me know if so.

I saw the same test failures existing in Travis, locally, on master, but was hoping it was just something with my environment. I'll take a closer look.

@jszwedko
Copy link
Contributor Author

I think the test failures are related to #99

@jszwedko
Copy link
Contributor Author

And are fixed by #96 so that PR may need to be merged ahead of this one to keep master green.

@pathawks
Copy link
Member

So the advantage would be that, if a user does not speak English and the browser cannot automatically redirect the user, some browser plugin might automatically translate the page into the users native language?

Makes sense.

@jszwedko
Copy link
Contributor Author

@pathawks the intent is to serve visitors with disabilities that rely on technology like screen readers to browse the web. I'm less familiar with automatic language translation for visual users, but I imagine that, yes, it may also assist with that.

@pathawks
Copy link
Member

@jszwedko I see where you're coming from and, while I don't disagree, I'm not sure what circumstances would cause the page to not redirect automatically.

@jszwedko
Copy link
Contributor Author

@pathawks the primary case in which this would happen is where the user does not have JS enabled (either by default or in general) -- I imagine this use case is also why there is link on the page for users to click. Given that there is text, I don't think it is unreasonable to set the language for the page 😄

@pathawks
Copy link
Member

No, not unreasonable.

I would note that, even when JS is disabled, the <meta> tag should still trigger a redirect.

@jszwedko
Copy link
Contributor Author

That is fair, missed that!

@@ -21,6 +21,7 @@ def initialize(site, base, dir, name)
def generate_redirect_content(item_url)
self.output = self.content = <<-EOF
<!DOCTYPE html>
<html lang="en-US">
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok if we add this. Could we terminate it on line 32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@jszwedko Did you consider using @benbalter's suggestion?

{{ site.lang | default: "en-US" }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parkr I did, but since the text on the page is English (regardless of what the site language is), it felt like en-US was the best fit. Curious to hear any other thoughts about this though.

@parkr
Copy link
Member

parkr commented Mar 22, 2016

@jszwedko Your tests need to be updated to account for this change.

WCAG 2.0, Guideline 3.1.1 requires that

"The default human language of each Web page can be programmatically
determined"
@jszwedko
Copy link
Contributor Author

@parkr doh, that's what I get for editing through the Github UI. I updated the test and squashed the commits down to one. Thanks for pointing that out!

@pathawks
Copy link
Member

:shipit:

@parkr
Copy link
Member

parkr commented Apr 14, 2016

🤷 ok!

@benbalter
Copy link
Contributor

benbalter commented Jun 22, 2016

@jekyllbot: merge

@jekyllbot jekyllbot merged commit dd96265 into jekyll:master Jun 22, 2016
jekyllbot added a commit that referenced this pull request Jun 22, 2016
@benbalter
Copy link
Contributor

Thanks @jszwedko! Sorry this took so long to merge 😄

@jszwedko
Copy link
Contributor Author

@benbalter no worries, thanks for getting it in!

@jekyll jekyll locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants