Reach-ui: Fix issues with how IDs are handled

[chaance]

🐛 Bug report

Current Behavior

There are a few places in the project where IDs are auto-generated on the client. This may be undesirable for a few reasons (users who disable JS lose important accessibility features, potential race condition bugs when used with non-React libs, two examples).

Further, in our auto-generated IDs we often use colons as delimiters. document.querySelector throws an error when receiving a string with a colon.

document.querySelector('#id:1')
SyntaxError: '#id:1' is not a valid selector

Expected behavior

We should 1) allow users to set IDs via props everywhere we are generating them currently, and 2) we should remove colons from our generated IDs.

[danieltott]

~@chancestrickland The first part makes sense, but can you point out where there are colons in generated IDs? I have found three components that use generated IDs: alert-dialog, menu-button, and skip-nav (although with skip-nav it's not generated, it's hard-coded). But it seems like when creating an ID, they're being concatenated with -, not : (labelId: `alert-dialog-${genId()}`, for example. I'm sure I'm missing something because I'm still getting my bearings in the source code.~ Sorry, please disregard. I got stuck on a hella-old fork from a while ago. 🤦‍♂️

I'd be happy to work on this one though; I'm all caught up now.

[chaance]

@danieltott Sounds good -- I created a new branch for this but that's as far as I've gotten. Happy to take a look when you have a PR ready.

[danieltott]

@chancestrickland For updating components to allow for ID overriding, would it make sense to create a series of PRs per Reach component? Or would you prefer a single, larger PR?

[chaance]

@danieltott Single PR is fine. We typically release all at once and only the updated packages will get a version bump.

[danieltott]

@chancestrickland What do you think about using an invariant call when a component id and an aria attribute that refers to that id don't line up? I'm handling a lot of cases automatically, but if we're allowing users to override attributes, then that case could still come up. Example would be <ComboboxButton aria-controls="someid" /> and <ComboboxInput id="someotherid" />. I'm automatically handling the case where a user sets id on ComboboxInput, but if they _also_ override aria-controls on ComboboxButton and it doesn't match up, that'll be an accessibility issue. Should we throw a warning/error via invariant for that case? Or just assume that if they're overriding that much stuff then they know they're risking messing things up?

[chaance]

@danieltott We try to handle attributes like aria-controls automatically, so even if the user passes their own ID to the input they shouldn't need to worry about its controller component. In this particular case we still need to allow the user to pass an ID for combobox (working on that one), but I don't see any value in allowing them to override aria-controls.

[danieltott]

@chancestrickland Sounds good. I should have a PR up early this week.