Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Sep 7, 2021

Summary

closes #5145 (NB: This bug only exists in 37.6.1, and should be fixed in 37.6.2)

#5130 updated the euiScreenReaderOnly mixin (which EuiCheckbox and EuiRadio both use) to include clip(0 0 0 0) CSS (to prevent scrolling issues caused by absolute positioning). Per the QA section of that PR, I tested the change against the normal checkbox/radio examples in our docs, but did not test them against checkbox/radios with no labels (which I hadn't realized functioned slightly differently than their counterparts with labels).

Checkboxes and radios without labels actually do not want the standard euiScreenReaderOnly behavior. The visually hidden input actually overlays the visually displayed element with a set height/width, unlike SR behavior which moves the hidden element totally offscreen.

As such, I decided the cleanest solution would be to only apply the euiScreenReaderOnly CSS to checkboxes/radios with labels (using :not()), thus negating the need for checkbox/radios to override any parts euiScreenReaderOnly CSS.

Before

After

Checkboxes/radios with labels remain the same:

QA

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for the any docs 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

@cee-chen cee-chen changed the title Screen reader only nolabel [EuiCheckbox][EuiRadio] Fix checkboxes/radios without labels not being clickable Sep 7, 2021
@cee-chen cee-chen marked this pull request as ready for review September 7, 2021 16:48
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5149/

opacity: 0; /* 1 */
z-index: 1; /* 1 */
margin: 0; /* 1 */
left: 0; /* 1 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you check out the "Before" screenshot, left: 0 was presumably added to override left: -10000px being set by euiScreenReaderOnly CSS. If you remove the CSS from being loaded at all on this input, there's no need to set left: 0 - position absolute will typically float the element where it would originally have been placed

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Also tested the other "checkable" components that use the mixin (EuiKeyPadMenuItem, EuiResizableCollapseButton) and saw no regression

@cee-chen
Copy link
Contributor Author

cee-chen commented Sep 7, 2021

@thompsongl Apologies for any wasted time on your part, @cchaos pinged me to let me know she'd rather revert #5130 for the next patch release. I'm going to close this PR, open a reversion PR, and then open a PR with #5130 plus this fix with a target release of the next breaking major (38.x).

CC @chandlerprall as a heads up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Eui table selection doesnt work in the last version

3 participants