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

fix: wrong or duplicate focusable #344

Merged
merged 3 commits into from
Nov 10, 2022
Merged

fix: wrong or duplicate focusable #344

merged 3 commits into from
Nov 10, 2022

Conversation

kecrily
Copy link
Member

@kecrily kecrily commented Sep 12, 2022 •
edited

  1. It is pointless to focus on these two places, they are not clickable.
1 2
截屏2022-09-12 13 32 37 截屏2022-09-12 13 32 11
  1. There are places where a class name called focusable exists, but there is no such style.
  2. Some svg's have two identical focusable=false attributes.

@eslint-github-bot eslint-github-bot bot added bug Something isn't working triage labels Sep 12, 2022
@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for es-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/es-eslint/deploys/6363467810b32a00089f8761
😎 Deploy Preview https://deploy-preview-344--es-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for zh-hans-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/zh-hans-eslint/deploys/636346781cd84c0008da7fca
😎 Deploy Preview https://deploy-preview-344--zh-hans-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for ja-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/ja-eslint/deploys/636346780de4f20007015310
😎 Deploy Preview https://deploy-preview-344--ja-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for new-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/new-eslint/deploys/636346781cd84c0008da7fc5
😎 Deploy Preview https://deploy-preview-344--new-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for hi-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/hi-eslint/deploys/63634678b007520008992e7a
😎 Deploy Preview https://deploy-preview-344--hi-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for de-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/de-eslint/deploys/63634678587e050008e4c6fd
😎 Deploy Preview https://deploy-preview-344--de-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for fr-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/fr-eslint/deploys/63634678dbedc10008407061
😎 Deploy Preview https://deploy-preview-344--fr-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Sep 12, 2022 •
edited

✅ Deploy Preview for pt-br-eslint ready!

Name Link
🔨 Latest commit 38ae99b
🔍 Latest deploy log https://app.netlify.com/sites/pt-br-eslint/deploys/636346789737500008d27adf
😎 Deploy Preview https://deploy-preview-344--pt-br-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

harish-sethuraman
harish-sethuraman reviewed Sep 14, 2022
View reviewed changes
src/_includes/components/testimonials-slider.html Outdated
@@ -2,7 +2,7 @@
<div class="c-slider__slides-container" data-slides>
<div class="c-slider__slides-wrapper" data-slides-wrapper>
{%- for item in site.donate_page.testimonials.items -%}
<div class="c-slider__slide focusable" data-slide>
<div class="c-slider__slide" data-slide>
Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2022-09-14 at 9 49 41 PM

I still see the focus in your deploy preview can you check this? or is this expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it out. I'm not sure which node is affecting this behavior right now, and I'm going to wake up tomorrow and check to see.

@nzakas
Copy link
Member

nzakas commented Sep 14, 2022

I think the first two might be intentional for easier keyboard navigation. Let me check with Sara.

@SaraSoueidan
Copy link
Contributor

SaraSoueidan commented Sep 15, 2022 •
edited

@kecrily Hey Percy!

Thanks so much for contributing to the a11y of the site!

Re focusability & clickability:

In general: it doesn't have to be clickable to be focusable. For example, scrollable regions must be focusable even though they're not "clickable" per se. Sometimes making a group focusable is useful for screen reader announcements.

  1. For the slider: focusing on the slide allows the screen reader to announce its contents without the user needing to manually navigate to its contents. This is sometimes helpful, particularly when there are no interactive elements within the slide. So this focusability is targeting screen reader users, not sighted keyboard or mouse users. (So it's not pointless.) That being said, it is not a requirement to keep it focusable. Removing it won’t break the user experience. So I have no strong opinion here. I’ll leave this to @nzakas to decide. (P.S. It is added from the JavaScript of the slider.)
  2. Re list of donations: Excellent catch. I had the tabindex="0" set on it because the design had the list of donations scroll horizontally on mobile. We eventually changed the design to make it more mobile-friendly. I must have forgotten to remove it (obviously). Great catch, thank you!

Re duplicate focusable="false" on <svg>s: Another great catch, thanks!

There are places where a class name called focusable exists, but there is no such style.

The :focus-visible polyfill will add this class name to elements that are focusable. I'm not sure which elements have this class name but have no focus styles. I'm thinking it's either something I have missed, or something I have deliberately not added focus styles to (for some reason; I'll need to know which elements have it to remember.)

nzakas
nzakas requested changes Nov 2, 2022
View reviewed changes
src/_includes/components/testimonials-slider.html Outdated
@@ -1,8 +1,8 @@
<div class="c-slider c-slider--testimonials" data-slider data-aria-label="Testimonials">
<div class="c-slider__slides-container" data-slides>
<div class="c-slider__slides-wrapper" data-slides-wrapper>
<div class="c-slider__slides-wrapper" tabindex="-1" data-slides-wrapper>
Copy link
Member

Choose a reason for hiding this comment

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

@kecrily based on @SaraSoueidan's comments, can we revert these changes in the slider?

nzakas
nzakas approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas nzakas merged commit a37e368 into main Nov 10, 2022
@nzakas nzakas deleted the fix/focusable branch November 10, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harish-sethuraman harish-sethuraman harish-sethuraman left review comments

@nzakas nzakas nzakas approved these changes

Assignees
No one assigned
Labels
bug Something isn't working triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kecrily @nzakas @SaraSoueidan @harish-sethuraman