[a11y] sub nav component contains nested interactive elements

[alexgibson]

Description

On mobile viewports, the sub navigation dropdown heading consists of two nested, interactive components (a link inside a button). This can cause issues for assisted technologies, and could also technically produce unexpected results in some browsers, since it's invalid HTML.

Steps to reproduce

1. Open https://www.mozilla.org/en-US/firefox/new/
2. Resize to mobile viewport.
3. Inspect the navigation menu drop down

<h2 class="c-sub-navigation-title is-summary">
  <button type="button" aria-controls="expand-csubnavigationcsubnavigationtitle-0" aria-expanded="false">
    <a href="/en-US/firefox/" data-link-type="nav" data-link-position="subnav" data-link-text="">
      <img class="c-sub-navigation-icon" src="https://www.mozilla.org/media/protocol/img/logos/firefox/logo.fedb52c912d6.svg" width="24" height="24" alt="">
        Firefox
    </a>
  </button>
</h2>

Expected result

There should only be one interactive component.

Actual result

Environment

N/A

[alexgibson]

Possible fixes for this:

1. We make it a rule that the navigation title should never be a link. This will mean we add a new sub-nav link item to each nav as a replacement for what is currently the title link:

The downside is we then have a "Firefox" repeated twice in the nav, but maybe that's OK? I always forget the title/icon is clickable.

2. We make it a link only on desktop, and have a separate hidden heading that we turn into a dropdown on mobile. This would make it look identical to what we have no visually, but adds some complexity / redundant markup that needs to be shown/hidden depending on viewport size.

[janbrasna]

Does it have to be a button, or a div with some interactive role better representing the markup output would suffice?

Does the content necessarily have to nest inside the button, or it can be outside of it structurally, but just styled in a way it behaves today — as if the link is in the foreground getting the events for its own hitzone only, and the button being in background underneath, receiving the dropdown hits outside of the actual link to manipulate the submenu…?

[alexgibson]

Does the content necessarily have to nest inside the button, or it can be outside of it structurally, but just styled in a way it behaves today — as if the link is in the foreground getting the events for its own hitzone only, and the button being in background underneath, receiving the dropdown hits outside of the actual link to manipulate the submenu…?

This is a good question! It uses the Protocol details component, which injects a <button> as the clickable element. But perhaps we don't actually need that here, and we could just use some custom JS instead. That's certainly a third option.

[alexgibson]

That's certainly a third option

Ok yeah, looking at the code perhaps this is the most straight forward path. We can just never inject a <button> inside the link to begin with, and avoid using that component over something a bit more custom.

[janbrasna]

The details component behaves rather nicely and has good a11y defaults, as compared to e.g. other collapsible menu instances: mozilla/protocol#947 — so would stick to it if possible, only moving things around.

The issue with interactive elements inside button roles is that AT turns that all into plaintext basically, so they can't be navigated. And it doesn't matter if that's button, or details' summary — any link inside these mostly won't work with AT (that basically don't "see" it there, or add it to the document outline for interaction).

So my thought was to keep the existing functionality, add some placeholder for the dropdown button, and move the category link outside — as it itself is not a part of the button/dropdown anyways, it only coincidentally shares the same physical space;) — and stack it with z-index to basically do what it does now.

I haven't looked into the implementation details honestly, but was thinking about changing the current:

h2
- button
-- a
--- (heading text)

into

h2
- a
-- (heading text)
- button
-- (new details summary text CTA ala "open ${heading_text} submenu" that would be visually hidden)

that would be placed below the link to act as the hitzone around it, but don't know how much of the mzp details magic is tied to the actual heading usage.

[alexgibson]

I'm not sure your suggestion can be implemented with the current details JS and the expectations it makes, but I'll take a closer look. I don't think there's too much additional a11y affordance needed for a simple drop down menu, if we did need to make something custom.

[alexgibson]

OK, I think I have a way to keep using details. It requires duplicating the <h2> and hiding / showing the right one based on viewport size, but that seems like a relatively low cost.

[janbrasna]

OK I quickly glanced where it is attached:

bedrock/media/js/base/protocol/protocol-sub-navigation.js

Line 10 in 51f89f0

var subNavTitle = '.c-sub-navigation .c-sub-navigation-title';

and it doesn't require the heading per-se. What about about leaving that h2.c-sub-navigation-title alone, instead adding a new helper element below it only serving to attach the details on mobile instead?

h2  (leave this alone)
- a
-- (heading text)
new element  (construct this on mq change and attach here?)
- button
- (visually hidden affordance text to also serve as labelledby aria)

to separate the h2 and title link from the details target (that can possibly be created by the JS as needed for smaller viewports?)

[alexgibson]

This is kind of what I'm already suggesting (but not generating the new element on the fly), but we're now nit-picking on semantics. I think on mobile, it should probably still be an <h2>, which would be better than a generic <div> or some other element. I am also trying to avoid design changes and new strings for translation if possible, so as to not over-complicate things.

Edit: hmm, but it would be nice to still have the original h2 link accessible on mobile (which is something that is not possible right now even in prod). Perhaps this is something I can construct in a better way.

[janbrasna]

Sorry, I haven't really tried changing it around to see how complicated it might be, this was just thinking aloud without any PoC at hand. The idea was to keep the h2 serving its purpose on all viewports, and only construct the details-expanding-element-target somehow "around" (=below?) that existing hitzone.

(Oversimplification: Left side h2: anchor, right side / remaining white space, basically the expand chevron affordance and then what's left in the space: the target for the details.)

That's basically what the current invalid markup does anyways. (I'm able to hit both the chevron and space around h2, as well as the h2 itself correctly in webkit):

trim.39D4CC16-4A3C-48D1-B3E2-AA0FF172D4B7.MOV

[alexgibson]

Ah, I see what you were meaning, thanks! I'll see what i can do here, but i also don't want to snowball this solution into something a lot more complicated. These sub nav menus will likely be overhauled over the coming year anyhow.

[janbrasna]

Ah, and I was thinking this interaction fix could have been the last thing needed before upstreaming that component to Protocol, that's why the extra 👀 eyes;) I wouldn't bother anyone otherwise.

[alexgibson]

No worries, I appreciate the thoughts and ideas!

[alexgibson]

@janbrasna I think I have something workable based on your suggestion, and i also managed to find a pre-translated string suitable for the toggle 🤞

[janbrasna]

Lovely! Will try it out in a bit… I never realised it would be more block elements taking up the same space so some kind of display mode trickery might be needed to accommodate for both hitzones overlapping… (but I never doubted you'd find a solution without too much extra markup, not for a second!;))

Q: Actually, maybe a completely wrong idea — could the extra element be added just in a before/after pseudo or it needs more specific targeting for the JS hooks than that? (EDIT: Ah okay it has a ftl translation inside anyways, nevermind…)

Praise: Thanks for converting all the submenus to the macro, I had it on my wishlist for some time too;)