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

Fix for focus appearing on mousedown of checkbox labels #2640

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

edwardhorsford
Copy link
Contributor

@edwardhorsford edwardhorsford commented Aug 3, 2020

Context

Quick for this issue. Because the filter has tabindex="-1", it briefly gets focus each mousedown on a checkbox label.

My fix involves having a second focusable element inside the one we already have. That element has focus styles suppressed. When a user clicks on a checkbox label, it receives focus, but nothing visually changes. When they release their cursor, the checkboxes are focused as normal.

As that second element has tabindex="-1" users can't normally tab in to it. As our main filter has normal focus styles, it looks correct when the popover is opened.

Before

filter-a-list-focus-bug-before

After

filter-a-list-focus-bug-after2

Note I've raised an issue upstream with the MOJ design system about the issue.

Other changes

I fixed the incorrect focus styling on the filter component.

@tijmenb tijmenb temporarily deployed to apply-for-te-fix-filter-grakzs August 3, 2020 15:15 Inactive
@adamsilver adamsilver self-requested a review August 3, 2020 15:24
Copy link
Contributor

@adamsilver adamsilver left a comment

Choose a reason for hiding this comment

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

Nice one - but the focus styles are needed for small screens.

Can you make sure the focus style works there and maybe only include the tabindex on that.

Bear in mind that the MOJ JS component can be configured to toggle the filter on big screens regardless. So the inclusion of the tabindex should be based on this too and not just screen size (matchmedia).

@edwardhorsford edwardhorsford marked this pull request as draft August 3, 2020 18:18
@tvararu tvararu added the ProVendor Provider or Vendor facing issue label Aug 4, 2020
@edwardhorsford
Copy link
Contributor Author

@adamsilver updated with my revised solution - the wider component can now show correct focus styles - but you won't get a weird flash when users click the checkbox.

@edwardhorsford edwardhorsford temporarily deployed to apply-for-te-fix-filter-grakzs August 4, 2020 14:34 Inactive
Copy link
Contributor

@duncanjbrown duncanjbrown left a comment

Choose a reason for hiding this comment

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

LGTM — one comment re comments!

@@ -36,7 +36,9 @@
</div>
<% end %>

<div class="moj-filter__options">
<!-- Div made focusable to 'catch' focus from checkboxes. see https://github.com/DFE-Digital/apply-for-teacher-training/pull/2640
Copy link
Contributor

@duncanjbrown duncanjbrown Aug 4, 2020

Choose a reason for hiding this comment

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

No need for this comment here or in the CSS — GitHub's merge commit will include the PR reference & we'll be able to get back to it from the diff. Possibly the rationale ought to go into the commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@duncanjbrown normally I'd agree - but in this case I think what I'm doing is unexpected and it might be helpful to note this is intentionally weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

As you like!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ProVendor Provider or Vendor facing issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants