Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Input: fix issues from a11y review #21205

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Input: fix issues from a11y review #21205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-react-input-80a0046e-4134-4acd-adfc-9e114bb26396.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Fix styling issues found in accessibility review",
"packageName": "@fluentui/react-input",
"email": "[email protected]",
"dependentChangeType": "patch"
}
10 changes: 10 additions & 0 deletions packages/react-input/src/components/Input/useInputStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ const useRootStyles = makeStyles({
// This is if the user clicks the field again while it's already focused
borderBottomColor: tokens.colorCompoundBrandStrokePressed,
},
':focus-within': {
Copy link
Member

Choose a reason for hiding this comment

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

Makes me wonder if we'll need a shorthand method for outline

Copy link
Member Author

@ecraig12345 ecraig12345 Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Filed #21231 -- I agree we should have a shorthand and thought about adding it in this PR, but figured it would be better to do separately in case it gets hung up on discussion for some reason (also I wasn't totally clear on how to set up the types from looking at other examples).

outlineWidth: '2px',
outlineStyle: 'solid',
outlineColor: 'transparent',
},
},
small: {
minHeight: fieldHeights.small,
Expand Down Expand Up @@ -167,15 +172,20 @@ const useRootStyles = makeStyles({
},
disabled: {
cursor: 'not-allowed',
backgroundColor: tokens.colorTransparentBackground,
...shorthands.border('1px', 'solid', tokens.colorNeutralStrokeDisabled),
...shorthands.borderRadius(tokens.borderRadiusMedium), // because underline doesn't usually have a radius
'@media (forced-colors: active)': {
...shorthands.borderColor('GrayText'),
Copy link
Member

Choose a reason for hiding this comment

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

Opinion: I wish the theme would handle this for us rather than having to ensure media queries in every disabled state for every component.

Copy link
Member Author

Choose a reason for hiding this comment

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

@smhigley should confirm but IIRC this would only be needed when we're not using semantic elements (in this case, the wrapper div is visually pretending to be the input)

Copy link
Member Author

Choose a reason for hiding this comment

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

But yeah, I agree that it would be nicer to have this done automatically

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it's only for when disabled colors are needed on non-semantic elements, or when we aren't using the native disabled attribute (e.g. disabledFocusable)

},
},
});

const useInputElementStyles = makeStyles({
base: {
boxSizing: 'border-box',
flexGrow: 1,
minWidth: 0, // required to make the input shrink to fit the wrapper
Copy link
Member

Choose a reason for hiding this comment

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

What about setting flexShrink?

Copy link
Member Author

Choose a reason for hiding this comment

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

It defaults to 1 (the desired value) but for some reason having a minWidth is also required to make it apply properly 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

it's b/c it appears there's some browser default for minWidth on text-like inputs that isn't exposed (you can verify by making a block input with no other styles, and shrinking the browser width down far enough)

...shorthands.borderStyle('none'), // input itself never has a border (this is handled by inputWrapper)
...shorthands.padding('0', horizontalSpacing.xxs),
color: tokens.colorNeutralForeground1,
Expand Down
35 changes: 25 additions & 10 deletions packages/react-input/src/stories/InputAppearance.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
import * as React from 'react';
import { Label } from '@fluentui/react-label';
import { useId } from '@fluentui/react-utilities';
import { makeStyles } from '@fluentui/react-make-styles';
import { makeStyles, shorthands } from '@fluentui/react-make-styles';
import { tokens } from '@fluentui/react-theme';
import { Input } from '../index';

const useStyles = makeStyles({
root: {
'& label': { display: 'block', paddingBottom: '2px' },
'& label:not(:first-child)': { paddingTop: '20px' },
// filledLighter and filledDarker appearances depend on particular background colors,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should this comment be above the filledDarker and filledLighter lines rather than the padding of divs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily--these wrapper divs are used in all cases but only have a background color defined for filled appearances

// so the story includes wrapper divs around the example of each appearance
'> div': { ...shorthands.padding('20px'), ...shorthands.borderRadius('20px') },
'> div:not(:first-child)': { paddingTop: '10px' },
},
filledDarker: { backgroundColor: tokens.colorNeutralBackground1 },
// ideally should match doc site, #faf9f8
filledLighter: { backgroundColor: tokens.colorNeutralBackground2 },
});

export const Appearance = () => {
Expand All @@ -20,17 +27,25 @@ export const Appearance = () => {

return (
<div className={styles.root}>
<Label htmlFor={outlineId}>Outline (default)</Label>
<Input appearance="outline" id={outlineId} placeholder="placeholder" />
<div>
<Label htmlFor={outlineId}>Outline (default)</Label>
<Input appearance="outline" id={outlineId} />
</div>

<Label htmlFor={underlineId}>Underline</Label>
<Input appearance="underline" id={underlineId} placeholder="placeholder" />
<div>
<Label htmlFor={underlineId}>Underline</Label>
<Input appearance="underline" id={underlineId} />
</div>

<Label htmlFor={filledLighterId}>Filled lighter</Label>
<Input appearance="filledLighter" id={filledLighterId} placeholder="placeholder" />
<div className={styles.filledLighter}>
<Label htmlFor={filledLighterId}>Filled lighter</Label>
<Input appearance="filledLighter" id={filledLighterId} />
</div>

<Label htmlFor={filledDarkerId}>Filled darker</Label>
<Input appearance="filledDarker" id={filledDarkerId} placeholder="placeholder" />
<div className={styles.filledDarker}>
<Label htmlFor={filledDarkerId}>Filled darker</Label>
<Input appearance="filledDarker" id={filledDarkerId} />
</div>
</div>
);
};
Expand Down
11 changes: 3 additions & 8 deletions packages/react-input/src/stories/InputContentBeforeAfter.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,13 @@ export const ContentBeforeAfter = () => {
return (
<div className={styles.root}>
<Label htmlFor={beforeId}>Content before</Label>
<Input contentBefore={<SearchRegular />} id={beforeId} placeholder="placeholder" />
<Input contentBefore={<SearchRegular />} id={beforeId} />

<Label htmlFor={afterId}>Content after</Label>
<Input contentAfter={<DismissRegular />} id={afterId} placeholder="placeholder" />
<Input contentAfter={<DismissRegular />} id={afterId} />

<Label htmlFor={bothId}>Content before and after</Label>
<Input
contentBefore={<SearchRegular />}
contentAfter={<DismissRegular />}
id={bothId}
placeholder="placeholder"
/>
<Input contentBefore={<SearchRegular />} contentAfter={<DismissRegular />} id={bothId} />
</div>
);
};
Expand Down
1 change: 0 additions & 1 deletion packages/react-input/src/stories/InputDefault.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const Default = (props: InputProps) => {
const argTypes: ArgTypes = {
// Add these native props to the props table and controls pane
placeholder: {
defaultValue: 'placeholder',
description:
'Placeholder text for the input. If using this instead of a label (which is ' +
'not recommended), be sure to provide an `aria-label` for screen reader users.',
Expand Down
6 changes: 3 additions & 3 deletions packages/react-input/src/stories/InputSize.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,17 @@ export const Size = () => {
<Label size="small" htmlFor={smallId}>
Small input
</Label>
<Input size="small" placeholder="placeholder" id={smallId} />
<Input size="small" id={smallId} />

<Label size="medium" htmlFor={mediumId}>
Medium input
</Label>
<Input size="medium" placeholder="placeholder" id={mediumId} />
<Input size="medium" id={mediumId} />

<Label size="large" htmlFor={largeId}>
Large input
</Label>
<Input size="large" placeholder="placeholder" id={largeId} />
<Input size="large" id={largeId} />
</div>
);
};
Expand Down