Fix/dropdown accessibility (#4977)

[tragessere]

Fix for issue #4977

This update allows passing through id and aria-controls props through AbstractNavItem to the resulting component.

It also adds an aria-controls attribute to the dropdown button referring to the menu in NavDropdown, DropdownButton, and SplitButton. The menus are now rendered in the DOM and hidden from the start, rather than being added after their first show. This is to allow setting aria-controls with a valid id at the start.

If the aria-controls attribute was intentionally not used I can remove it again.

[taion]

cc @jpreynat this would also fix #4954

we always want to set aria tags, as long as they're not redundant. do we really want to always render the dropdown items unconditionally, though? making this the default behavior seems like it would constitute a breaking change. perhaps it could be opt-in?

[jpreynat]

@taion Indeed, this would resolve my issue 👍
I think that both behaviors are deeply linked, since rendering the whole HTML is mandatory to allow navigation through aria-controls.

But for sure, this would be a breaking change in regards to the current behavior of react-bootstrap.
I would recommend making it opt-in for now, and maybe switch it to opt-out in a future major release?

[taion]

In practice this is unlikely to break anyone. Also, I think this matches what's down in upstream TWBS. And we're still in betas, so strict semver compliance is not a concern. cc @jquense thoughts?

[jquense]

A few things.

I don't think aria-controls is actually supported by any SRs, or wasn't a few months ago. I'd be reticent to rely on it as a fix.

The lazy rendering of dropdowns is an optimization but I don't think it was a premature one. We should some git and issue archaeology before removing it. React land is different than normal bootstrap, the lazily rendered tree isn't just HTML here, it's the component tree, with all it's logic like modals etc.

I could be misremembering, but I think that lazy rendering was added in response to a real perf bottleneck

[tragessere]

Looks like you're right on the support, I hadn't realized it was mostly being ignored at this point. It was turned off in JAWS by default in April 2019 (w3c/aria#995), though NVDA might be looking to support it at some point (nvaccess/nvda#8983).

From the JAWS patch notes

If you encounter an element on a web page with a defined ARIA controls relationship, JAWS will no longer say "use JAWSKEY+ALT+M to move to controlled element" by default. In most cases, the target of the controls relationship is adjacent to the element or does not provide any useful information.

The dropdown menu here is a sibling to the button which seems to make the aria-controls redundant anyway. Along with the performance concerns, there doesn't seem to be any benefit to be had anymore. I'll go ahead and remove the aria-controls related changes.

[tragessere]

As far as optionally rendering the nav dropdown. I could add a prop to NavDropdown, maybe renderBeforeShow?, that passes through to DropdownMenu to decide whether to render the menu before it's shown the first time. Not sure if you'd prefer to extend this pull request to cover that use case still, or if it should be separate.

[jquense]

I think it'd be fine to add a prop like that to opt out of the logic. We should maybe separately think through what the default should be there going forward

[tragessere]

There is now a prop on DropdownMenu to allow rendering it in the DOM initially, as well as a prop in NavDropdown, DropdownButton, and SplitButton to pass through to the DropdownMenu. I've left it disabled by default to maintain current behavior and to avoid having performance issues come up with default usage.

[taion]

btw, this is related to #3940 – @jquense do you have any thoughts on the right thing to do here?

[taion]

shouldn't the explicitly-specified values take precedence over calculated ones?

[jquense]

hmm normally yes, but in this case, if there is a context provided id the user would have to provide the id in both places otherwise you get a mismatch and it's broken. We used to warn saying the ID is ignored, and that one should use generateId or whatever it is on the parent to control ids

[taion]

what should we do about #3940, then?

[jquense]

maybe just warn again?

[taion]

hmm, would this actually fix that issue? because the navcontext won't assign a controller ID?

[jquense]

we should always use the controller id, might just want to surface to a user that we are ignoring their props id?

[tragessere]

@taion That's correct, in that issue the getControllerId function for ListGroupItem will be a noop function giving an undefined id, so this would instead default to the user provided value.

@jquense I know you approved the changes earlier, but I just added a warning message when the user's id is overwritten here like you mentioned. I updated the Tabs component to pass the id prop through to the inner tabs so that this component can know whether the user provided an id in that case. (but it's overwritten, so no change in behavior) I used the warning library for this, so technically it appears as a console error. Let me know if there are any changes you'd like me to make to that, or if it was fine before and I can revert it.

[taion]

Thanks!!!