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 issues #1447

Closed
wants to merge 11 commits into from
Closed

Accessibility issues #1447

wants to merge 11 commits into from

Conversation

lfarrell
Copy link
Contributor

@lfarrell lfarrell commented Dec 7, 2023

Fix for: #1395
Fix for #1393
Fix for #1430

Also addresses color contrast issues and duplicate ids in search results

Should fix all accessibility issues in arclight.

@lfarrell lfarrell changed the title Draft: Accessibility issues Accessibility issues Dec 8, 2023
@randalldfloyd randalldfloyd self-requested a review December 8, 2023 14:54
@seanaery seanaery self-requested a review December 8, 2023 18:40
<div class='breadcrumb-links'>
<%= rendered_breadcrumbs %>
</div>
<dt>
Copy link
Contributor

Choose a reason for hiding this comment

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

I like where you're going with it, but as far as I understand it, a <dd> can't be a child of <dt>. The missing indentation makes it a bit confusing. I wonder if a small tweak like this would work:

<dt class="visually-hidden">Collection Context</dt>
<dd>...</dd>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanaery That seems to work. I've pushed up the change.

@@ -29,10 +29,12 @@
// "view more"/"view less" text swaps when content is expanded
.view-more {
display: inline;
font-size: 1.5rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks too big in the UI -- it makes the view more/less link bigger than the title header. I'm curious what the accessibility violation is with the existing style. Looks like it's --bs-link-color which translates to #0D6EFD which is 4.5:1 on white. If it's used on any non-white backgrounds I think it'd be better to darken it in those contexts (can even use BS5
Screenshot 2023-12-08 at 3 08 40 PM
shade-color() functions).

@@ -22,10 +22,6 @@ $hierarchy-view-collapse-icon: url("data:image/svg+xml,<svg xmlns='http://www.w3
}
}

.al-hierarchy-highlight > .documentHeader {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave this in -- I think it is useful to visually highlight the currently selected item. And now that you have unlinked the current highlighted item, its text color is black so there's no contrast issue to resolve on the highlighted document.

@seanaery
Copy link
Contributor

seanaery commented Dec 8, 2023

This is looking great and I am excited to get all these fixes into core. These are big important wins -- thanks for working through all of this.

I completed a review and left a few inline comments. Just a couple more thoughts, taking an updated look at the branch:

  1. I believe we should keep the background-color highlight on the currently-viewed item in the sidebar; I think it's especially helpful if you have a collection with a deep hierarchy and you're trying to navigate between different component nodes at different levels.
  2. There's a lot here in one PR with 10+ commits, some of which undo or supplement the effects of others. When this gets merged it'd be a shame to squash this all into one big "Accessibility issues" commit, but leaving it un-squashed would also leave a bit of a confusing paper trail.

I wonder if you might either 1) break the changes from this PR out into 5-7 smaller separate PRs; or alternatively 2) make a copy of the branch, do an interactive rebase, e.g. $ git rebase -i HEAD~15 and edit down / reorder the commit history with fixup, pick, reword, etc. so each of the final fixes here has a clear paper trail, and then post that branch for one new PR.

@lfarrell lfarrell closed this Dec 11, 2023
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.

2 participants