Aligning switch semantics with aria role

[myasonik]

Summary

Kibana was seeing issues with switches. Though we weren't able to figure out the root cause of the issue, switches have a unique aria role that they weren't taking advantage of. So, instead of continuing to dig, I thought I'd update the semantics skirting the issue a bit.

Following Inclusive Component's recommendation I've moved switches away from <input type=checkbox /> to <button role=switch aria-checked=true|false>.

• Made sure the deprecation flow of shimming the event object works in Chrome/FF/Safari/IE 11/ChromiumEdge

Checklist

• Checked in dark mode
• Checked in mobile
• Checked in IE11 and Firefox
  - [ ] Props have proper autodocs
  - [ ] Added documentation examples
• Added or updated jest tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[myasonik]

Firefox doesn't use outline for focus states; this removes the border firefox gives all focused elements

[cchaos]

The only visual regression I'm seeing is that the height of the whole control is now more than 20px probably from some extra browser default styles coming with <button>. Can you see if you can get this back down to 20px?

Before

This PR

[thompsongl]

Haven't tracked down why this happens, but in Firefox only:

Before:

After:

Lost some animation smoothness.

[thompsongl]

You'll need to move your entries to the master section at the top on the file.
(We're working on automating this somehow so it's not an annoyance in the future)

[myasonik]

Not sure I did correct still...

[myasonik]

@thompsongl Hmmm, it seems pretty smooth on my machine...

[cchaos]

@myasonik I think you might have forgotten to push your latest changes?

[cchaos]

The style & SASS changes LGTM, but if we remove onChange for onToggle and cause this to be a breaking change, there's still work to do.

[timroes]

Is there a specific reason (I didn't find any comment about that) why onChange has been renamed to onToggle? This feels very unrelated to fixing the semantics of the underlying HTML element and also has the problem (despite deprecation), that we need to fix A LOT OF code in Kibana. Also having every control using the same change callback name had the nice side effect that you knew it was onChange and you don't need to dig into documentation, to figure out if it's onChange, onToggle, onChecked, etc. depending on the concrete type?

[myasonik]

@timroes It was a bit talked about here.

Basically:

1. Method name sounded more accurate
2. Having different method names lets you at a glance know if it's the deprecated API or the new one
3. Seems to give the cleanest deprecation flow (if using the old method name, shim the event object to give the expected checked attribute; else, use the new method signature)

I don't think anyone held any strong feelings on this though. So, if you think keeping the same method name is better, I can revert the name change. Would you shim the event all the time then? Or not do it at all?

[timroes]

Good question, I am also not strongly against renaming it, just wondering if we shouldn't then do a similar renaming for the other controls as well, so they are not using onChange (unless they are really input fields)? So if we would keep onChange I assume we should also keep the "shimmed" event. But I don't have too strong feeling in either direction, so I'll totally leave the decision to you.

[myasonik]

@chandlerprall got any strong feelings?

I'm leaning towards leaving it how I currently have it (deprecated onChange with shimmed event object, new onToggle method).

Not sure what to do about other components though. Nothing right now but, in the future... 🤷‍♂

[chandlerprall]

It definitely is nice to keep the form controls' API the same and expose onChange, but in this case I think that becomes disingenuous as the onChange for switch no longer matches the other form controls unless we always shim the checked prop onto the button, which doesn't seem like the right long-term choice.

Which brings up another point - the shimming of a checked onto the button won't enable backwards compatibility in TypeScript.

This PR updates EuiSwitchProps to ButtonHTMLAttributes<HTMLInputElement> & { but it should be ButtonHTMLAttributes<HTMLButtonElement> & {. With that additional change, accessing the change event's target.checked in a typescript file fails

Error:(10, 49) TS2339: Property 'checked' does not exist on type 'EventTarget & HTMLButtonElement'.

We would need to typedef the shim with

	  export type EuiSwitchProps = CommonProps &
	    Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'onChange'> & {
	      label?: ReactNode;
	      checked: boolean;
	      onChange?: (event: React.FormEvent<HTMLButtonElement & { checked: boolean }>) => void;
	      onToggle?: Function;
	      disabled?: boolean;
	      compressed?: boolean;
	    };

[chandlerprall]

Alright, we discussed this in the design meeting and we're all leaning toward maintaining the better developer experience, which is keeping this as onChange with no deprecation.

This assumes:

• All browsers are fine with setting checked to true/false on a button element
• TypeScript defs in EUI can communicate this additional value correctly (see my previous comment)
• React's synthetic event wrapping doesn't do anything weird to prevent this

[myasonik]

OK, I've removed the breaking change and have left the shimed checked property.

I also changed the documentation to not actually use that property anymore though, instead relying on locally toggling the state, to quietly discourage its use. (Most implementations of swtich seem to do this anyway.)

[cchaos]

Styling lgtm

[thompsongl]

I'm still seeing the choppy animation in Firefox, but if no one else can reproduce, then everything looks good.