Rename SkipLinkController to FocusController, update unit tests & add story
#12555
Conversation
Manage this branch in SquashTest this branch here: https://lb-cleanuprename-skip-link-con-14jdc.squash.io |
0852fc5 to
26fbe6d
Compare
26fbe6d to
49e73e2
Compare
49e73e2 to
291b386
Compare
|
It's just a migration from the original JS that was there (links below). My understanding is that while a plain link can work OK, it's a better experience with the focus outline being triggered. In all the sites I've worked on, there's always been a JS behaviour attached (link or buttons vary). Update, it's so that the fragment doesn't update in the URL also, which adds unwanted history. For context:
More than happy for us to remove the JS completely but we'd probably have to consider other implications of this and get some accessibility team input first. |
|
Just updated the comment above. |
291b386 to
f5ce45a
Compare
f5ce45a to
a3b18c1
Compare
|
I have updated this PR to pull out the In addition, I have fixed up some issues with the |
|
CI failure is unrelated to this PR. |
| const selector = | ||
| this.targetValue || this.element.getAttribute('href') || 'main'; | ||
|
|
||
| this.target = |
There was a problem hiding this comment.
Note. We should move this logic to the focus method. And add a unit test for the focus target appearing after initial render.
There was a problem hiding this comment.
Might be nice to add a unit test for an element created at the same time as the focus is clicked. This is why we want to defer by a delay. Maybe using the clone controller in the unit test.
| button.focus(); | ||
| expect(document.activeElement).toBe(button); | ||
|
|
||
| expect(element.getAttribute('tabindex')).toBe(null); |
There was a problem hiding this comment.
Add another test for an element 'just created'.
- Ensure Jest only adapter items are in adapter.js - Ensure Jest AND Storybook items are in stubs.js - Add mock for someElement.scrollIntoView in the adapter
- Update the Controller import & identifier to be `w-focus` not `w-skip-link` - Rework methods to be easier to maintain, pull out the focus event attachment handler to a function - Add JSDoc examples & refine description - Only add then remove the tabindex attribute if needed - Rework unit tests to be the emerging common structure, add unit test coverage & test for the preservation of existing tabindex attributes
a3b18c1 to
0757a19
Compare
|
@Srishti-j18 I have updated this PR to include a better approach to the Feel free to incorporate this however you need to get your PR working reliably, we can clean up your PR later once this PR is in. I have asked for a review on this within the core team but I am not sure when that will happen. |
Description
The
SkipLinkControllerwas one of the first controllers implemented and as such has not adopted some of the practice that's emerged through other Stimulus adoption work.Details
This PR introduces the following:
@exampletagsforceFocusas we use this in lots of other place in the codeSee also #12554 (a similar, in purpose, rename & clean up effort).
How to test
Within the admin interface, once refreshed, press
tab. The first target element should be the 'skip to main content' link, activating this (press enter) should move your focus away and into the main content. Pressingtabagain, should focus on the first focusable element in the main content (e.g. the first link '34 pages' in the screen below).Why this matters
Eventually we want to consider broader usage of the focus controller, either made available through documentation or for 'scroll to top' type buttons around lo 8000 nger page content. This is already a request for a different issue - #12128
This also makes it easier for us to eventually centralise the Jest testing for Stimulus controllers, the
await setuppattern is super useful and makes adding future unit tests much easier.Finally, the fact that the element's name (Link) is in the controller is a bit of an anti-pattern, any element could be used (e.g. a button) and should normally not be reflected in the controller name.
While renames seem unnecessary, without this rename it may seem like we need a different controller do do any 'focus' work, when in fact Stimulus shines when we create smaller modular 'behaviour' controllers and not usage specific ones. This aligns with our Stimulus tips, which were not really full formed when this controller was created. https://docs.wagtail.org/en/stable/contributing/ui_guidelines.html#helpful-tips