[InputBase] Pass aria attributes to input #14580

ghost · 2019-02-19T11:55:59Z

I have followed (at least) the PR section of the contributing guide.

~~Hey, added fix for aria attributes being passed to div rather than input.~~

Breaking change

The props provided to the Input, FilledInput, OutlinedInput, and InputBase are now spread on the native input element instead of the root element (div).

This is the behavior most of our users expect. We are not following the root spread convention for hopefully, a more intuitive API. Many people have been complaining about the InputProps vs inputProps difference (different case) #10064. This problem is gone. We will deprecate inputProps in v4.1, it's a soft deprecation for now.

eps1lon

This is a working solution to the immediate problem. However it adds more technical debt to the inputProps <-> InputProps ambiguity. Maybe we can remove them and spread excess props in InputBase to the input rather than the div. I don't think many people expect or value the current behavior. For styling we could always use some wrapper class.

oliviertassinari · 2019-02-19T16:22:09Z

@eps1lon Are you suggesting the introducing of a rootProps property? It's an exception to the rule, but It's indeed simpler! I wish we could remove this wrapping div.

eps1lon · 2019-02-19T16:28:53Z

@eps1lon Are you suggesting the introducing of a rootProps property?

I would just omit it for now. Are people actually interested in it?

oliviertassinari · 2019-02-19T16:36:33Z

@eps1lon I have no idea. Ok, I'm happy with this plan of action.

packages/material-ui/src/InputBase/InputBase.js

oliviertassinari · 2019-03-26T18:38:37Z

#1578 (comment)

mui-pr-bot · 2019-03-29T17:06:54Z

@material-ui/core: parsed: -0.19% 😍, gzip: -0.28% 😍

Details of bundle changes.

Comparing: eb357a9...a074ad6

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.19%	-0.28%	350,243	349,572	90,024	89,770
@material-ui/core/Paper	0.00%	0.00%	67,867	67,867	19,823	19,823
@material-ui/core/Paper.esm	0.00%	0.00%	60,198	60,198	18,568	18,568
@material-ui/core/Popper	0.00%	0.00%	30,463	30,463	10,527	10,527
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,384	17,384	5,729	5,729
@material-ui/core/useMediaQuery	0.00%	0.00%	2,469	2,469	1,041	1,041
@material-ui/lab	0.00%	0.00%	148,279	148,279	43,580	43,580
@material-ui/styles	0.00%	0.00%	53,099	53,099	15,433	15,433
@material-ui/system	0.00%	0.00%	17,136	17,136	4,525	4,525
Button	0.00%	0.00%	87,947	87,947	26,064	26,064
Modal	0.00%	0.00%	82,055	82,055	24,563	24,563
colorManipulator	0.00%	0.00%	3,232	3,232	1,299	1,299
docs.landing	0.00%	0.00%	49,958	49,958	10,837	10,837
docs.main	-0.06%	-0.08%	645,407	645,022	200,517	200,352
packages/material-ui/build/umd/material-ui.production.min.js	-0.22%	-0.32%	298,542	297,873	82,758	82,491

Generated by 🚫 dangerJS against a074ad6

oliviertassinari · 2019-03-29T18:15:52Z

@eps1lon I have tried the prop spread change from root to input. I'm not 💯% convinced it's a better idea.

eps1lon

"root component" has nothing to do with to top component but rather where excess props are spread and the root class is applied.

root component being equal to the top component was incidental.

The breaking change is that the root component is now an input instead of a div.

eps1lon · 2019-03-29T22:51:16Z

packages/material-ui/src/test-utils/describeConformance.js

@@ -76,7 +76,12 @@ function testComponentProp(element, getOptions) {
 function testPropsSpread(element, getOptions) {
  it(`does spread props to the root component`, () => {
    // type def in ConformanceOptions
-    const { classes, inheritComponent, mount } = getOptions();
+    const { classes, inheritComponent, mount, propsSpread = true } = getOptions();


The way you would want to implement this is with a skip option. The current implementation would still create a test report.

Do you mean that we should return earlier (before it())?

I don't understand what you mean by skip.

Use a skip option to filter in

https://github.com/mui-org/material-ui/blob/71234113e3d14bcbfe0d3acec3023d219615d74e/packages/material-ui/src/test-utils/describeConformance.js#L136

The filter should also include testComponentPropWith so that we can get rid of
https://github.com/mui-org/material-ui/blob/71234113e3d14bcbfe0d3acec3023d219615d74e/packages/material-ui/src/test-utils/describeConformance.js#L57

packages/material-ui/src/Select/SelectInput.js

packages/material-ui/src/InputBase/InputBase.js

eps1lon · 2019-03-29T23:01:20Z

packages/material-ui/src/Select/SelectInput.test.js

@@ -41,7 +42,7 @@ describe('<SelectInput />', () => {

  it('should render a correct top element', () => {
    const wrapper = shallow(<SelectInput {...defaultProps} />);
-    assert.strictEqual(wrapper.name(), 'div');
+    assert.strictEqual(wrapper.name(), 'Fragment');


We might as well remove it. The test does not provide any value. Nobody cares what the "top element" is. We don't have any public API about what a "top element" is.

eps1lon · 2019-03-29T23:02:54Z

packages/material-ui/src/InputBase/InputBase.test.js

@@ -38,6 +38,7 @@ describe('<InputBase />', () => {
    classes,
    inheritComponent: 'div',
    mount,
+    propsSpread: false,


That would be a breaking change. I don't understand how this relates to passing through of aria attributes.

The relation comes from an old comment of you: #14580 (review).

But that's not what I said. I wanted to change to root component from the div to the input.

You have introduced a new concept: a top component. So, you would like to keep the current changes but to push them one step further? .root -> .container, .input -> .root?

A few components don't have any top component, like the Tooltip. The component doesn't have any root classes.

oliviertassinari · 2019-03-30T12:58:24Z

The breaking change is that the root component is now an input instead of a div.

@eps1lon It depends on each word definitions.

oliviertassinari · 2019-03-30T13:20:46Z

In the following, we gonna use your new concept: the top component, it represents the first element in the DOM structure. It might not be present when using a fragment. The root component is where we spread the properties

There is one more technicality to take into account, the NativeSelectInput and SelectInput internal components share the same style sheet but have a different DOM & React structure. We can take a few components that are a good illustration of the possible edge cases: Tooltip, ListItem, InputBase, Select, Button.

I see two options:

We make the CSS API matches the DOM API as much as possible. We apply the .root class name on the top element.

Tooltip: no .root class as the top element is a fragment.
ListItem: most the time, the top element is a li, we add an extra .container class when the top element changes.
Button: a simple case, the .root is applied on the top element
InputBase: the .root class is applied on the top div.
Select: there is no top element. The .root has no place

We make the CSS API matches the React API as much as possible. We apply the .root class on the root element.

Tooltip: the .root class is applied on a the child element.
ListItem: the root element is a li, we apply root on the li, we add an extra .container class when the top element changes.
Button: a simple case, the .root is applied on the root element
InputBase: the .root class is applied on the inner input, we add an extra .container class for the top div element.
Select: there is no top element. The root element change depending on the implementation. We change the root application.

oliviertassinari · 2019-03-30T13:22:26Z

I have been using the strategy n°1. so far, with this switch of definitions:

top element -> root element
root element -> spread element.

oliviertassinari · 2019-03-30T13:27:11Z

@eps1lon So I have two questions, with your definitions:

Do you think that we should we change the root element of the InputBase?
Do you think that we should we follow the strategy 1. or 2.?

mbrookes · 2019-03-30T20:26:06Z

@eps1lon

#14580 (review)

"root component" has nothing to do with to top component but rather where excess props are spread and the root class is applied.

root component being equal to the top component was incidental.

My understanding is that the root element is (currently) defined as the top or outermost element in a component, and the class applied to this was named root to match this definition. Spreading excess props to the root element was done for predictability.

From the docs:

Name	Type	Default	Description
`component`	`Component`	`'span'`	The component used for the root node. [...]

and:

"Any other properties supplied will be spread to the root element <element>."

Whether that is always intuitive is a different question. I'm not objecting to any particular proposed solution, including a change in terminology if it makes sense.

eps1lon · 2019-04-03T20:46:10Z

Do you think that we should we change the root element of the InputBase?

To be honest I would prefer to be strict here and not allow any special cases. Every excess prop of InputBase is spread to the root component. For everything else we already have inputProps. At least if you want to stick with the "root component = component that renders the outermost host".

My definition "root = whatever we define" gives us more flexibility. Excess props and root class would be applied to the input and the wrapper would just get a wrapper class and WrapperProps would be spread to it. The downside is that "root" is misleading in a local context (maybe switch to "base component"?).

oliviertassinari added the accessibility a11y label Feb 19, 2019

oliviertassinari approved these changes Feb 19, 2019

View reviewed changes

oliviertassinari requested a review from eps1lon February 19, 2019 15:57

oliviertassinari changed the title ~~[InputBase]: Pass aria attributes to input~~ [InputBase] Pass aria attributes to input Feb 19, 2019

eps1lon reviewed Feb 19, 2019

View reviewed changes

vitkon reviewed Mar 19, 2019

View reviewed changes

packages/material-ui/src/InputBase/InputBase.js Outdated Show resolved Hide resolved

oliviertassinari added the breaking change label Mar 28, 2019

oliviertassinari self-assigned this Mar 29, 2019

Michal Sidoruk and others added 2 commits March 29, 2019 11:33

[InputBase]: Pass aria attributes to input

9f8239d

fix ci

cd7627b

oliviertassinari added breaking change and removed breaking change labels Mar 29, 2019

oliviertassinari requested a review from eps1lon March 29, 2019 18:13

BC, soft deprecation of inputProps

a074ad6

oliviertassinari approved these changes Mar 29, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label Mar 29, 2019

oliviertassinari removed their assignment Mar 29, 2019

eps1lon requested changes Mar 29, 2019

View reviewed changes

oliviertassinari removed the PR: good for merge label Mar 29, 2019

oliviertassinari self-assigned this Mar 30, 2019

oliviertassinari mentioned this pull request Apr 1, 2019

[docs] Add ref forwarding to API docs #15135

Merged

oliviertassinari removed their assignment Apr 21, 2019

oliviertassinari mentioned this pull request Apr 23, 2019

Select is not getting vertically aligned with TextFields #15447

Closed

2 tasks

oliviertassinari force-pushed the next branch from ad52812 to 5a768d7 Compare April 28, 2019 10:54

oliviertassinari self-assigned this May 3, 2019

oliviertassinari mentioned this pull request May 3, 2019

[material-ui][TextField] InputProps vs inputProps confusion #11377

Open

1 task

oliviertassinari removed their assignment May 3, 2019

oliviertassinari changed the base branch from next to master May 23, 2019 21:10

oliviertassinari mentioned this pull request Jun 11, 2019

[Select] Fix specificity issue #16137

Merged

1 task

oliviertassinari closed this Jun 13, 2019

oliviertassinari mentioned this pull request Aug 9, 2019

use of min/max and endAdornment in InputProps #16952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InputBase] Pass aria attributes to input #14580

[InputBase] Pass aria attributes to input #14580

ghost commented Feb 19, 2019 •

edited by oliviertassinari

eps1lon left a comment

oliviertassinari commented Feb 19, 2019 •

edited

eps1lon commented Feb 19, 2019

oliviertassinari commented Feb 19, 2019

oliviertassinari commented Mar 26, 2019

mui-pr-bot commented Mar 29, 2019 •

edited

oliviertassinari commented Mar 29, 2019

eps1lon left a comment

eps1lon Mar 29, 2019

oliviertassinari Mar 29, 2019

oliviertassinari Mar 30, 2019

eps1lon Mar 31, 2019 •

edited

eps1lon Mar 29, 2019

oliviertassinari Mar 29, 2019 •

edited

eps1lon Mar 29, 2019

oliviertassinari Mar 29, 2019 •

edited

eps1lon Mar 30, 2019

oliviertassinari Mar 30, 2019 •

edited

oliviertassinari Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

mbrookes commented Mar 30, 2019 •

edited

eps1lon commented Apr 3, 2019

[InputBase] Pass aria attributes to input #14580

[InputBase] Pass aria attributes to input #14580

Conversation

ghost commented Feb 19, 2019 • edited by oliviertassinari

Breaking change

eps1lon left a comment

Choose a reason for hiding this comment

oliviertassinari commented Feb 19, 2019 • edited

eps1lon commented Feb 19, 2019

oliviertassinari commented Feb 19, 2019

oliviertassinari commented Mar 26, 2019

mui-pr-bot commented Mar 29, 2019 • edited

oliviertassinari commented Mar 29, 2019

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon Mar 29, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 29, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 30, 2019

Choose a reason for hiding this comment

eps1lon Mar 31, 2019 • edited

Choose a reason for hiding this comment

eps1lon Mar 29, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 29, 2019 • edited

Choose a reason for hiding this comment

eps1lon Mar 29, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 29, 2019 • edited

Choose a reason for hiding this comment

eps1lon Mar 30, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 30, 2019 • edited

Choose a reason for hiding this comment

oliviertassinari Mar 30, 2019 • edited

Choose a reason for hiding this comment

oliviertassinari commented Mar 30, 2019 • edited

oliviertassinari commented Mar 30, 2019 • edited

oliviertassinari commented Mar 30, 2019 • edited

oliviertassinari commented Mar 30, 2019 • edited

mbrookes commented Mar 30, 2019 • edited

eps1lon commented Apr 3, 2019

ghost commented Feb 19, 2019 •

edited by oliviertassinari

oliviertassinari commented Feb 19, 2019 •

edited

mui-pr-bot commented Mar 29, 2019 •

edited

eps1lon Mar 31, 2019 •

edited

oliviertassinari Mar 29, 2019 •

edited

oliviertassinari Mar 29, 2019 •

edited

oliviertassinari Mar 30, 2019 •

edited

oliviertassinari Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

oliviertassinari commented Mar 30, 2019 •

edited

mbrookes commented Mar 30, 2019 •

edited