fix: Making the hidden input on Checkbox and Switch only cover the indicator and not also the label by khmakoto · Pull Request #24638 · microsoft/fluentui

khmakoto · 2022-09-01T19:23:58Z

Current Behavior

The hidden input in the Checkbox and Switch components covers both the indicator and the label, making any interactive elements within the label, like links, impossible to be clicked.

Input

Label

New Behavior

The hidden input in the Checkbox and Switch components now covers only the indicator and its surrounding space, not the label. This makes any interactive elements within the label, like links, possible to be clicked. We verified that clicking on the label still toggles the state of the component. We also expanded the size of the label so it covers the empty space around it that was previously being covered by the hidden input.

We verified that hover and pressed styling still works as expected and we have added visual regression tests that check for RTL to work correctly as well.

Input

Label

Related Issue(s)

Fixes #24330

…dicator and not also the label.

behowell

This change will also make it so the padding around the control, and the gap between the control and the label are dead zones, but they are required to be part of the hit target by the spec (and in general for usability).

Interactive content should probably be outside the label? But if it's necessary to do it this way, then we should consider refactoring Checkbox/Switch/Radio so that the <label> is the root element, wrapping the indicator and label text.

We should make sure that Radio also works the same way as Checkbox and Switch.

fabricteam · 2022-09-01T19:33:54Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-switch` Switch	`32.097 kB` `10.27 kB`	`33.534 kB` `10.635 kB`	`1.437 kB` `365 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`188.681 kB` `52.366 kB`
`react-components` react-components: FluentProvider & webLightTheme	`33.394 kB` `11.007 kB`
`react-portal-compat` PortalCompatProvider	`5.851 kB` `1.964 kB`

_{🤖 This report was generated against 8f38bb63574e31bf2fa9f2f1d14b166d6c1a627f}

codesandbox-ci · 2022-09-01T19:34:09Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f2dac77:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2022-09-01T19:36:20Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 8f38bb63574e31bf2fa9f2f1d14b166d6c1a627f (build)

fabricteam · 2022-09-01T19:42:31Z

Perf Analysis (`@fluentui/react-components`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1300	1257	5000
Button	mount	939	934	5000
FluentProvider	mount	1478	1510	5000
FluentProviderWithTheme	mount	581	564	10
FluentProviderWithTheme	virtual-rerender	544	540	10
FluentProviderWithTheme	virtual-rerender-with-unmount	563	575	10
MakeStyles	mount	1934	1975	50000
SpinButton	mount	2324	2341	5000

smhigley

I can't tell what the difference is in the screener change and the update looks fine, so seems good to me :)

smhigley · 2022-09-02T21:05:10Z

re: @behowell's comment on dead zones -- I agree it'd be better if there weren't, though it doesn't concern me all that much, since the indicator itself combined with the label text itself should end up being a pretty big hit target.

A wrapping label would be nice though :). I forget if there was a reason for not doing that originally for checkbox/switch.

msft-fluent-ui-bot · 2022-09-10T00:00:04Z

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

…heckboxSwitchInputOnlyCoversIndicator

…icked.

…ked.

packages/react-components/react-checkbox/src/components/Checkbox/useCheckboxStyles.ts

behowell · 2022-09-28T20:31:06Z

packages/react-components/react-checkbox/src/components/Checkbox/useCheckboxStyles.ts

+  // Use a (negative) margin to account for the difference between the indicator's height and the label's line height,
+  // as well as accounting for the introduced padding for handling the "empty space" around the label.
+  // This prevents the label from expanding the height of the checkbox, but preserves line height if the label wraps.
  medium: {
-    marginTop: `calc((${indicatorSizeMedium} - ${tokens.lineHeightBase300}) / 2)`,
-    marginBottom: `calc((${indicatorSizeMedium} - ${tokens.lineHeightBase300}) / 2)`,
+    marginTop: `calc(-1 * ${tokens.spacingVerticalS} + (${indicatorSizeMedium} - ${tokens.lineHeightBase300}) / 2)`,
+    marginBottom: `calc(-1 * ${tokens.spacingVerticalS} + (${indicatorSizeMedium} - ${tokens.lineHeightBase300}) / 2)`,
  },
  large: {
-    marginTop: `calc((${indicatorSizeLarge} - ${tokens.lineHeightBase300}) / 2)`,
-    marginBottom: `calc((${indicatorSizeLarge} - ${tokens.lineHeightBase300}) / 2)`,
+    marginTop: `calc(-1 * ${tokens.spacingVerticalS} + (${indicatorSizeLarge} - ${tokens.lineHeightBase300}) / 2)`,
+    marginBottom: `calc(-1 * ${tokens.spacingVerticalS} + (${indicatorSizeLarge} - ${tokens.lineHeightBase300}) / 2)`,
  },


IMO it would be better to remove the padding from root and put it on the individual slots instead of adding padding to both the label AND the root, then adding a negative margin to overlap it.

Agreed and done!

packages/react-components/react-checkbox/src/components/Checkbox/useCheckboxStyles.ts

…tive margins and adding RTL VR tests.

behowell

Looks good, thanks for making all of these changes to address my feedback!

behowell · 2022-09-28T23:20:27Z

packages/react-components/react-checkbox/src/components/Checkbox/useCheckboxStyles.ts

+  medium: {
+    width: `calc(${indicatorSizeMedium} + ${tokens.spacingHorizontalS})`,
+  },
+  large: {
+    width: `calc(${indicatorSizeLarge} + ${tokens.spacingHorizontalS})`,
+  },


I think you can get rid of spacingHorizontalS now that the label is using padding instead of margin for the gap, right?

Suggested change

medium: {

width: `calc(${indicatorSizeMedium} + ${tokens.spacingHorizontalS})`,

},

large: {

width: `calc(${indicatorSizeLarge} + ${tokens.spacingHorizontalS})`,

},

medium: {

width: indicatorSizeMedium,

},

large: {

width: indicatorSizeLarge,

},

No, that spacing is to account for the space on the left of the indicator (right if labelPosition is before). You can see that another tokens.spacingHorizontalM has been removed already that referred to the gap between the indicator and the label.

behowell · 2022-09-28T23:24:55Z

packages/react-components/react-switch/src/components/Switch/useSwitchStyles.ts

+
+    // Calculate the width of the hidden input by taking into account the size of the indicator + the padding around it.
+    // This is done so that clicking on that "empty space" still toggles the switch.
+    width: `calc(${trackWidth}px + ${tokens.spacingHorizontalS})`,


I haven't read the switch styles in as much detail, but similar to my comment here in checkbox, does this still need spacingHorizontalS included in it?

Suggested change

width: `calc(${trackWidth}px + ${tokens.spacingHorizontalS})`,

width: `${trackWidth}px`,

Yup, similar reason to the one above.

behowell · 2022-09-28T23:25:09Z

packages/react-components/react-switch/src/components/Switch/useSwitchStyles.ts

+  },
+  above: {
+    bottom: 0,
+    height: `calc(${trackHeight}px + ${tokens.spacingVerticalS})`,


Ditto here too

Suggested change

height: `calc(${trackHeight}px + ${tokens.spacingVerticalS})`,

height: `${trackHeight}px`,

khmakoto · 2022-09-29T01:14:36Z

@microsoft-github-policy-service agree [company="Microsoft"]

khmakoto · 2022-09-29T01:14:56Z

@microsoft-github-policy-service agree company="Microsoft"

…dicator and not also the label (microsoft#24638) * fix: Making the hidden input on Checkbox and Switch only cover the indicator and not also the label. * Adding change files. * Making empty space around indicator and label toggle checkbox when clicked. * Making empty space around indicator and label toggle switch when clicked. * Using indicator + label paddings instead of double paddings with negative margins and adding RTL VR tests. * Addressing cases where no label is provided for components. Co-authored-by: Humberto Makoto Morimoto Burgos <makotom@microsoft.com> Co-authored-by: KHMakoto <humberto_makoto@hotmail.com>

Humberto Makoto Morimoto Burgos added 2 commits September 1, 2022 12:10

fix: Making the hidden input on Checkbox and Switch only cover the in…

65a2345

…dicator and not also the label.

Adding change files.

ce7f6dd

khmakoto added Component: Checkbox Fluent UI react-components (v9) Component: Switch labels Sep 1, 2022

khmakoto self-assigned this Sep 1, 2022

khmakoto requested review from a team and behowell as code owners September 1, 2022 19:23

github-actions bot added this to the July Project Cycle Q3 2022 milestone Sep 1, 2022

behowell suggested changes Sep 1, 2022

View reviewed changes

msft-fluent-ui-bot added the Needs: Author Feedback label Sep 1, 2022

smhigley approved these changes Sep 2, 2022

View reviewed changes

msft-fluent-ui-bot added the Status: No Recent Activity label Sep 10, 2022

khmakoto removed Status: No Recent Activity Needs: Author Feedback labels Sep 10, 2022

Merge branch 'master' of https://github.com/microsoft/fluentui into c…

f5ff1ad

…heckboxSwitchInputOnlyCoversIndicator

github-actions bot modified the milestones: July Project Cycle Q3 2022, September Project Cycle Q1 2022 Sep 26, 2022

Humberto Makoto Morimoto Burgos and others added 2 commits September 26, 2022 17:50

Making empty space around indicator and label toggle checkbox when cl…

54a8998

…icked.

Making empty space around indicator and label toggle switch when clic…

ab408bc

…ked.

khmakoto requested a review from behowell September 27, 2022 23:46

behowell reviewed Sep 28, 2022

View reviewed changes

Using indicator + label paddings instead of double paddings with nega…

2501238

…tive margins and adding RTL VR tests.

khmakoto requested a review from a team as a code owner September 28, 2022 23:10

behowell reviewed Sep 28, 2022

View reviewed changes

behowell approved these changes Sep 28, 2022

View reviewed changes

Addressing cases where no label is provided for components.

f2dac77

khmakoto enabled auto-merge (squash) September 29, 2022 01:07

khmakoto closed this Sep 29, 2022

auto-merge was automatically disabled September 29, 2022 01:08
Pull request was closed

khmakoto reopened this Sep 29, 2022

khmakoto enabled auto-merge (squash) September 29, 2022 01:08

khmakoto merged commit e44263f into microsoft:master Sep 29, 2022

khmakoto deleted the checkboxSwitchInputOnlyCoversIndicator branch September 29, 2022 01:46

khmakoto mentioned this pull request Sep 29, 2022

[Bug] react-radio: interactive items within the label are unreachable with a pointer #25021

Closed

	width: `calc(${trackWidth}px + ${tokens.spacingHorizontalS})`,
	width: `${trackWidth}px`,

	height: `calc(${trackHeight}px + ${tokens.spacingVerticalS})`,
	height: `${trackHeight}px`,

Conversation

khmakoto commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Behavior

Input

Label

New Behavior

Input

Label

Related Issue(s)

Uh oh!

behowell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabricteam commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

codesandbox-ci bot commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smhigley left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smhigley commented Sep 2, 2022

Uh oh!

msft-fluent-ui-bot commented Sep 10, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

behowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khmakoto commented Sep 29, 2022

Uh oh!

khmakoto commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

khmakoto commented Sep 1, 2022 •

edited

Loading

behowell left a comment •

edited

Loading

fabricteam commented Sep 1, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 1, 2022 •

edited

Loading

size-auditor bot commented Sep 1, 2022 •

edited

Loading

fabricteam commented Sep 1, 2022 •

edited

Loading

smhigley left a comment •

edited

Loading