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.

Sign up for GitHub

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

Jump to bottom

Add lang attribute to <html> tag for accessibility improvement. #9664

Merged
merged 7 commits into from
Aug 9, 2024
Merged

Add lang attribute to <html> tag for accessibility improvement. #9664

merged 7 commits into from
Aug 9, 2024

Conversation

Jash2606
Copy link
Contributor

@Jash2606 Jash2606 commented Jul 31, 2024 •
edited
Loading

Closes #9634

Description

This PR adds a lang attribute to the <html> tag to specify the primary language of the document. This attribute is crucial for accessibility, as it helps screen readers and other assistive technologies interpret the language correctly, ensuring proper pronunciation and comprehension for users. The lang attribute has been set to "en" for English, as this is the primary language of the content.

Testing

  1. Open the modified HTML document in a browser.
  2. Inspect the <html> tag to confirm the presence of the lang="en" attribute.
  3. Use a screen reader or accessibility tool to verify that it recognizes the language of the document as English.

Stakeholders

@cdrini

@Jash2606
Copy link
Contributor Author

Jash2606 commented Aug 3, 2024

Hey @scottbarnes @cdrini , I have updated the lang attribute as mentioned in issue #9634.
Please review the changes and let me know if any further updates are needed.
Thank you!

@scottbarnes
Copy link
Collaborator

scottbarnes commented Aug 3, 2024 •
edited
Loading

Thanks for this, @Jash2606. I noticed #9634 has the label Needs: Detail, so I started a conversation there in the hope we could figure out how to proceed with this issue. As things stand, I am not convinced this PR is fully responsive to the issue raised. However, more discussion may help tease out what, if anything, should be done.

The PR has been updated so my original comment no longer reflects the current state of the PR.

@scottbarnes
Copy link
Collaborator

Thanks for this, @Jash2606. Just a heads up that you probably want to add this change to openlibrary/openlibrary/templates/site/head.html.

I just skimmed the code and I can't actually tell if this particular sitemap function is being used, but I imagine if the proposed change makes sense in head.html, it probably makes sense in sitemap.py as well.

But either way, please test out the changes you've made, e.g., as I did with curl, and show them in the PR so it's easier to review.

@cdrini cdrini mentioned this pull request Aug 6, 2024
Open
8 tasks
@Jash2606
Copy link
Contributor Author

Jash2606 commented Aug 6, 2024

Thanks for this, @Jash2606. Just a heads up that you probably want to add this change to openlibrary/openlibrary/templates/site/head.html.

I just skimmed the code and I can't actually tell if this particular sitemap function is being used, but I imagine if the proposed change makes sense in head.html, it probably makes sense in sitemap.py as well.

But either way, please test out the changes you've made, e.g., as I did with curl, and show them in the PR so it's easier to review.

Hey @scottbarnes ,

Changes done as suggested for head.html also !

@rebecca-shoptaw
Copy link
Collaborator

Hi @Jash2606! I had a look at the info in the issue, and this seems like a good approach to me.

I'd second what @scottbarnes said and suggest you test via curl to confirm it's working as expected. Once you get your Docker container up and running you can try the following:

curl -s http://localhost:8080 | head -n 5

The site should now default to lang="en" and look like this:

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="en">

You'll also want to confirm that the language is correctly switched if the bot selects a language, by running:

curl -s -H "Accept-Language: es" http://localhost:8080 | head -n 5

The site should still switch to lang="es" and look like this:

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="es">

Let me know how it goes! I'll try testing on my end as well, but it would be good to document your own test results in the PR. 🙂

@rebecca-shoptaw
Copy link
Collaborator

rebecca-shoptaw commented Aug 6, 2024 •
edited
Loading

@Jash2606 Update: I pulled the PR up on GitPod, and it looks like the code is going to need a slight reword!

Here's what it looks like in the dev tools currently:
New html lang screenshot

It looks like this is just happening because you forgot the curly braces in the move from sitemap to head; once those are added it should work correctly. 🙂

Tried adding in the curly braces on my end, and confirmed:
✅ curl with no language header returns lang="en"
✅ curl with a Spanish language header returns lang="es"

@rebecca-shoptaw rebecca-shoptaw added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Aug 6, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Aug 6, 2024
@Jash2606
Copy link
Contributor Author

Jash2606 commented Aug 6, 2024

@Jash2606 Update: I pulled the PR up on GitPod, and it looks like the code is going to need a slight reword!

Here's what it looks like in the dev tools currently: New html lang screenshot

It looks like this is just happening because you forgot the curly braces in the move from sitemap to head; once those are added it should work correctly. 🙂

Tried adding in the curly braces on my end, and confirmed: ✅ curl with no language header returns lang="en" ✅ curl with a Spanish language header returns lang="es"

Hey @rebecca-shoptaw ,

Updated Syntax changes , Let me know if any further changes required.

rebecca-shoptaw
rebecca-shoptaw approved these changes Aug 6, 2024
View reviewed changes
Copy link
Collaborator

@rebecca-shoptaw rebecca-shoptaw left a comment

Choose a reason for hiding this comment

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

@Jash2606 Great, thank you, and thanks for your great work on this!

Tested via GitPod:
✅ curl with no specified language defaults to lang="en"

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="en">
<head>

✅ curl with a language specified uses that language

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" lang="es">
<head>

Also, for your next PR I'd recommend having a look at the Git Cheat Sheet -- you'll find it much easier to keep things up to date and avoid manual merges if you create a new branch for each PR, and rebase off the master branch if needed. 🙂

@rebecca-shoptaw rebecca-shoptaw added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Aug 6, 2024
cdrini
cdrini reviewed Aug 9, 2024
View reviewed changes
@cdrini cdrini merged commit 5302bd9 into internetarchive:master Aug 9, 2024
4 checks passed
Souvik-Cyclic pushed a commit to Souvik-Cyclic/openlibrary that referenced this pull request Aug 17, 2024
@Jash2606 @Souvik-Cyclic
Add lang attribute to <html> tag for accessibility improvement. (inte…
a0ef70b 
…rnetarchive#9664)

Co-authored-by: Jash2606 <jash.23bcs10163@sst.scaler.com>
SivanC pushed a commit to SivanC/openlibrary that referenced this pull request Aug 20, 2024
@Jash2606 @SivanC
Add lang attribute to <html> tag for accessibility improvement. (inte…
15b01c1 
…rnetarchive#9664)

Co-authored-by: Jash2606 <jash.23bcs10163@sst.scaler.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdrini cdrini cdrini left review comments

@rebecca-shoptaw rebecca-shoptaw rebecca-shoptaw approved these changes

Assignees

@rebecca-shoptaw rebecca-shoptaw

Labels
Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

element does not have a [lang] attribute
4 participants
@Jash2606 @scottbarnes @rebecca-shoptaw @cdrini