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

Accessibility fixes #60

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Accessibility fixes #60

merged 2 commits into from
Feb 16, 2024

Conversation

cjcolvar
Copy link

@cjcolvar cjcolvar commented Feb 13, 2024

Changes in this PR:

Not resolved in this PR:

  • Demo site: Hidden element has focusable content #56
    • I believe MaterialUI handles the modal switching the app in the background to aria-hidden=true and adds custom JS handling to make the app's elements be non-focusable (https://mui.com/material-ui/react-modal/#focus-trap). We could add some more custom handling which would set and remove tabindex="-1" on all of the interactive elements in the app but that might be hacky given that MaterialUI also handles setting elements tabindex. (Maybe it is possible to override the methods in MaterialUI which set aria-hidden on the app root div node to also mark/unmark the tabindex on interactive elements?)

@cjcolvar
Copy link
Author

Thinking about this a little more I think we could work around the issue with not being able to modify the template used for generating the index and app/index pages. Since we're not changing those pages content often we could make copies of the generated pages, edit them, and then use them to overwrite the generated pages during the docker container build process or earlier in the build process.

@cjcolvar
Copy link
Author

I overrode the generated pages at the end of the build process and was able to insert the page language and modify the viewport. With this SiteImprove is happier (#56 still isn't handled) and the UI and behavior doesn't appear to be any different for me on Firefox. Since overflow is hidden I wonder if declaring a maximum-scale would be good since it would announce to the user that content gets hidden when zoomed above that threshold. Setting that would flag in SiteImprove so maybe what we really need to do is set overflow to scroll and not set a maximum-scale. I tried this and it seems like it didn't negatively impact normal 100% zoom. My tests were in the standalone demo site and things may behave differently in Avalon.

…s; Allow scrollbars on page for higher zoom percentages
@cjcolvar
Copy link
Author

I updated the PR description to reflect the changes I pushed in the last commit and mentioned in my previous comment.

Copy link

@Dananji Dananji left a comment

Choose a reason for hiding this comment

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

👍

I just have one question on the redirect to app/index.html.

all URLs for media files may work.
</p>
</div>
<a href="/app/index.html" class="button">Go to Timeliner</a>
Copy link

Choose a reason for hiding this comment

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

Does this need to point to the override code?

Copy link
Author

Choose a reason for hiding this comment

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

These files are copied into /dist/index.html at the end of yarn build so the path is correct.
https://github.com/IUBLibTech/timeliner/pull/60/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R10

@cjcolvar cjcolvar merged commit 743fa86 into main Feb 16, 2024
@cjcolvar cjcolvar deleted the a11y_fixes branch February 16, 2024 18:35
@elynema
Copy link

elynema commented Feb 20, 2024

@cjcolvar I'm still getting a message about a button missing a text alternative for the volume slider component from the SiteImprove browser plugin:

image.png

This seems like it would be quick / easy to resolve?

@elynema
Copy link

elynema commented Feb 20, 2024

Given what is documented for the volume slider here: #61, it may make sense to get rid of this button altogether, and so may not make sense to create a label for it.

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

Successfully merging this pull request may close these issues.

3 participants