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

Jump to bottom

[docs] Fix most lighthouse a11y issues in input demos #15780

Merged
merged 17 commits into from May 27, 2019
Merged

[docs] Fix most lighthouse a11y issues in input demos #15780

merged 17 commits into from May 27, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented May 21, 2019

Should fix all reported problems from lighthouse except color contrast issues. This is likely an issue with the design spec and needs more investigation. With a dark theme no contrast issues are reported for the components/inputs/* demos and we reach a 100 score for a11y. This is only for issues that are testable automatically. I didn't go through the manual tests.

This PR (db4228d in particular) might have some overlap with #15333.

@eps1lon eps1lon added docs Improvements or additions to the documentation accessibility a11y labels May 21, 2019
@eps1lon eps1lon requested a review from mbrookes May 21, 2019 19:43
@mui-pr-bot
Copy link

mui-pr-bot commented May 21, 2019 •
edited

Details of bundle changes.

Comparing: ca382ef...cb394d0

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 315,924 315,924 86,498 86,498
@material-ui/core/Paper 0.00% 0.00% 67,923 67,923 20,182 20,182
@material-ui/core/Paper.esm 0.00% 0.00% 61,205 61,205 18,969 18,969
@material-ui/core/Popper 0.00% 0.00% 28,740 28,740 10,346 10,346
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,376 2,376
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,575 1,575
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,781 5,781
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 975 975
@material-ui/lab 0.00% 0.00% 138,861 138,861 42,642 42,642
@material-ui/styles 0.00% 0.00% 51,406 51,406 15,199 15,199
@material-ui/system 0.00% 0.00% 14,458 14,458 4,181 4,181
Button 0.00% 0.00% 83,869 83,869 25,459 25,459
Modal 0.00% 0.00% 20,344 20,344 6,681 6,681
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing -0.04% -0.04% 55,991 55,970 14,061 14,056
docs.main -0.00% -0.00% 645,653 645,635 203,144 203,140
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 294,841 294,841 83,987 83,987

Generated by 🚫 dangerJS against cb394d0

@oliviertassinari oliviertassinari changed the base branch from next to master May 23, 2019 21:07
@oliviertassinari oliviertassinari self-assigned this May 24, 2019
oliviertassinari
oliviertassinari reviewed May 24, 2019
View reviewed changes
docs/src/pages/components/autocomplete/IntegrationDownshift.js Outdated
}),
label: 'Label',
},
inputProps: getInputProps(),
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to break the focused state. Focus the input, blur it. The focus state remains. I haven't dive into why.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't dive into why.

You shouldn't. I'm proposing the change so I should fix it.

Copy link
Member

Choose a reason for hiding this comment

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

I can think of two possible paths to fix it: one is in the core (chain event listener callbacks), one is in the demos (only forward the /aria-/ props). I'm wondering if the latter is the best one?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to re-evaluate that component (InputBase). There's too much going on there with props filtering i.e. what prop is passed to which component. Some changes might fall under bug or breaking change depending on the viewpoint. Will follow up on this.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, should we remove the change of the auto complete from this pull request so we can move forward with the others? (I need to look at the other changes).

Copy link
Member Author

Choose a reason for hiding this comment

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

What's wrong with the current approach? Focus handling should work now as of 4c0e285

@oliviertassinari oliviertassinari removed their assignment May 24, 2019
@eps1lon eps1lon force-pushed the docs/a11y-audit branch 2 times, most recently from b1b0959 to 4c0e285 Compare May 24, 2019 15:15
mbrookes
mbrookes previously requested changes May 24, 2019
View reviewed changes
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Mostly cosmetic...

docs/src/pages/components/autocomplete/IntegrationReactSelect.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationReactSelect.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationDownshift.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationAutosuggest.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationAutosuggest.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationReactSelect.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationReactSelect.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationDownshift.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationDownshift.js Outdated Show resolved Hide resolved
docs/src/pages/components/autocomplete/IntegrationDownshift.js Outdated Show resolved Hide resolved
oliviertassinari
oliviertassinari reviewed May 25, 2019
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited

Choose a reason for hiding this comment

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

The react-select and downshift demos don't work.

docs/src/pages/components/autocomplete/IntegrationDownshift.tsx Outdated
fullWidth: true,
classes,
label: 'countries',
InputLabelProps: getLabelProps({ strink: true } as any),
Copy link
Member

Choose a reason for hiding this comment

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

It's not needed, we can remove the strink prop.

docs/src/pages/components/autocomplete/IntegrationReactSelect.tsx
},
InputLabelProps: {
htmlFor: 'react-select-single',
shrink: true,
Copy link
Member

Choose a reason for hiding this comment

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

It's not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Either this or we remove the placeholder. Without this the placeholder is hidden.

@eps1lon
Copy link
Member Author

eps1lon commented May 25, 2019

The react-select and downshift demos don't work.

Could you be more specific?

@oliviertassinari
Copy link
Member

@eps1lon What's the nominal use case for these components? Does the default use case work? (tip: selecting a value among multiple)

@eps1lon
Copy link
Member Author

eps1lon commented May 26, 2019

Behavior between master and branch should be on par. There are still some issues but the scope of this PR is lighthouse score only. We can expand on this later because it could be a limitation of the 3rd party libraries.

oliviertassinari
oliviertassinari reviewed May 26, 2019
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited

Choose a reason for hiding this comment

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

Here is a demonstration of the problem with the last downshift demo. The expected result is to be able to clear the field once a value is selected.

May-26-2019 18-10-39

docs/src/pages/components/autocomplete/IntegrationDownshift.tsx Outdated
clearSelection();
}
}) => {
const { onBlur, onFocus, ...inputProps } = getInputProps({ foo: 1 });
Copy link
Member

Choose a reason for hiding this comment

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

foo?

Copy link
Member

Choose a reason for hiding this comment

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

Spotted with https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-15780--material-ui.netlify.com%2Fcomponents%2Fautocomplete%2F.

@eps1lon
Copy link
Member Author

eps1lon commented May 26, 2019

So it's the 4th. The 3rd has the same behavior on master. I'll have a look.

oliviertassinari
oliviertassinari approved these changes May 27, 2019
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@eps1lon Well done!

@eps1lon eps1lon merged commit 8cfaa98 into mui:master May 27, 2019
@eps1lon eps1lon deleted the docs/a11y-audit branch May 27, 2019 14:56
@eps1lon eps1lon mentioned this pull request May 27, 2019
Merged
@oliviertassinari oliviertassinari mentioned this pull request Jun 11, 2019
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@mbrookes mbrookes mbrookes left review comments

Assignees
No one assigned
Labels
accessibility a11y docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eps1lon @mui-pr-bot @oliviertassinari @mbrookes