search

mi6 / ic-ui-kit

Intelligence Community UI Kit (based on StencilJS)
MIT License
24 stars 25 forks source link

Using slots in top navigation title link causes focus issue #1508

Open GCHQ-Developer-847 opened 7 months ago

GCHQ-Developer-847 commented 7 months ago

Summary of the bug

When passing custom elements to the "app-title" and "app-icon" slots in a top navigation component, both elements are focussable but, when focussed, are within the same focus indicator, so it appears to a keyboard user as though they have to tab twice before their focus moves onto the next element on the page.

🪜 How to reproduce

  1. Go to the ICDS website, or the top navigation code example demonstrating the use of slots.
  2. Tab until the focus indicator appears on the app title link on the top navigation.
  3. Tab again.
  4. See how it looks like your focus has not moved (you have to tab twice in order to move onto the next element on the page).

📸 Screenshots or code

This is because, when you slot in a title (or short title) and icon, you have to give each one an href in order for both the icon / image and the text to be clickable and work like a link. So they both become focussable.

But :focus-within is used, so focussing on either shows the same surrounding focus indicator.

🖥 📱 Device

🧐 Expected behaviour

It should only require one tab press to move the visible focus away from the title link.

📝 Acceptance Criteria

Given that I am on a page which uses slots in the title link for the top navigation, When I tab to it, Then I should only need to tab one more time for my visible focus to move onto the next focussable element on the page.

Additional info

I would say this is probably a very minor issue (unless there are any WCAG criteria which this would fail) as the user can easily move their focus onto the next focussable element on the page by pressing tab again.

GCHQ-Developer-847 commented 7 months ago

Ideas for a possible solution

Having had a quick look into this, I have come up with a possible solution which would be to tell developers to just pass a normal image to the app-icon slot (so not an <a>, or NavLink, or anything with an href) and then we add padding and a negative margin to the <a slot="app-title"> element so that it covers the app-icon icon / image area, like this:

So wherever they click - on the text, or on the image, or even in between, it will still always navigate them to the link address.

We could possibly avoid a breaking change by only applying that padding and margin if the app-icon element is not an <a>.

Note: Depending on if we go with this solution or not, we may need to update the code example with slots and make the change to the top nav on the ICDS website. (Should probably update our Storybook examples as well).