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

Demo of possible fix for checkbox label focus glitch #1907

Closed
wants to merge 6 commits into from

Conversation

edwardhorsford
Copy link
Contributor

This PR demonstrates a possible fix to #1906.

I don't necessarily expect it to get merged - but the review app can help demonstrate how it works and possibly be used for testing.

I've tried it in Voiceover on mac and it seems to have no impact.

As it's fiddling with focus it would want to be well tested - but you can't directly focus these items anyway so it mostly makes changes only when mouse-down on a label.

@edwardhorsford
Copy link
Contributor Author

This fix is a variation of what I did to fix a similar issue on my service - there using a parent element as a 'trap' for the focus.

I actually think it's better if the checkbox input retains visual focus, so I prefer this approach.

One other approach could be a pure js solution looking for mousedown on a label and moving focus.

@owenatgov
Copy link
Contributor

Hi @edwardhorsford. After 2+ years someone's getting back to you on this 😅

Thanks for investigating this, the solution looks good. It has however prompted me to investigate. the issue and I think I've found a slightly neater solution that involves targeting the label with :active. By changing the focus state CSS that starts on on line 123 of the checkbox sass file so that it also targets .govuk-checkboxes__label:active:before, it applies the correct focus styling when the label is clicked ("active") even though focus technically isn't on the checkbox input anymore, as per your hypothesis.

I've done some very rudimentary testing and it seems to work well but I'm also interested in your opinion on this, if you can remember working on this. Depending on if you think this solution works we can do a couple of things to move it forward:

  1. Tidy up this PR to reflect the active solution
  2. Close this PR and open a new one with the active solution

Let me know what you think.

@edwardhorsford
Copy link
Contributor Author

@owenatgov thanks for looking at this.

I didn't immediately remember it specifically, but do recall fixing a bug related to this on our service. As my memory is patchy, it might be better if you just open a new pr with your solution. I'd be happy to give it a test though.

@owenatgov
Copy link
Contributor

I've made a PR with my proposal #2827

As this PR is recorded in the new PR, I'm going to close this one. We can always reopen this one if we discover that it's a more suitable solution.

@owenatgov owenatgov closed this Sep 5, 2022
@edwardhorsford edwardhorsford deleted the patch-2 branch September 5, 2022 14:12
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.

4 participants