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.

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

[fields] Use the PickersTextField component in the fields #10649

Merged
merged 248 commits into from
Feb 22, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 12, 2023

Closes #6384
Closes #10699
The design part is handled in #10778

PR extracted

Changelog

  • 🎁 Introduce a new DOM structure for the field components that provides a better accessibility

Breaking changes

  • The selectedSections prop no longer accepts start and end indexes.
    When selecting several β€” but not all β€” sections, the field components were not behaving correctly, you can now only select one or all sections:

     <DateField
    -  selectedSections={{ startIndex: 0, endIndex: 0 }}
    +  selectedSections={0}
    
       // If the field has 3 sections
    -  selectedSections={{ startIndex: 0, endIndex: 2 }}
    +  selectedSections="all"
     />
  • The headless field hooks (e.g.: useDateField) now return a new prop called enableAccessibleFieldDOMStructure.
    This is used to know if the current UI expected is built using the accessible DOM structure or not. Learn more about this new accessible DOM structure.

When building a custom UI, you are most-likely only supporting one DOM structure, so you can remove enableAccessibleFieldDOMStructure before it is passed to the DOM:

  function MyCustomTextField(props) {
    const {
+     // Should be ignored
+     enableAccessibleFieldDOMStructure,

      // ... rest of the props you are using
    }

    return ( /* Some UI to edit the date */ )
  }

  function MyCustomField(props) {
    const fieldResponse = useDateField<Dayjs, false, typeof textFieldProps>({
      ...props,
+     // If you only support one DOM structure, we advise you to hardcode it here to avoid unwanted switches in your application
+     enableAccessibleFieldDOMStructure: false,
    });

    return <MyCustomTextField ref={ref} {...fieldResponse} />;
  }

  function App() {
    return <DatePicker slots={{ field: MyCustomField }} />;
  }
  • The following internal types were exported by mistake and have been removed from the public API:

    • UseDateFieldDefaultizedProps
    • UseTimeFieldDefaultizedProps
    • UseDateTimeFieldDefaultizedProps
    • UseSingleInputDateRangeFieldComponentProps
    • UseSingleInputTimeRangeFieldComponentProps
    • UseSingleInputDateTimeRangeFieldComponentProps

@flaviendelangle flaviendelangle added breaking change component: pickers This is the name of the generic UI component, not the React module! v7.x labels Oct 12, 2023
@flaviendelangle flaviendelangle self-assigned this Oct 12, 2023
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 16, 2023
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 3, 2023
@flaviendelangle flaviendelangle changed the base branch from master to next November 6, 2023 14:05
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 7, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 13, 2023

Ideas on this direction:

  • Tests. Looking at the changes in the PR to our tests, and test helpers, it seems that developers will too have to write their tests differently. What's the implication for end-to-end tests? Do we need to create documentation on how to create tests?
  • Tab behavior. We could consider the input a composite widget. I would expect some designers to complain that it's hard to quickly move between fields in a complex form if a date picker takes 4 tabs to move to the next one vs. 1 like the Autocomplete. So maybe to consider a prop to be able to customize this behavior.
  • Customizability. Will previous customization break? I guess not. Will it be simpler to customize the input? I guess it will since we have access to more DOM nodes to apply styles.

@flaviendelangle
Copy link
Member Author

Tests. Looking at the changes in the PR to our tests, and test helpers, it seems that developers will too have to write their tests differently.

People should still be able to fire an onChange event on the input element.
That's not how people interact with it on a real application, but it's something we work hard to provide because it the test required to press each key individually it would be a nightmare.
So nothing changes on this topic compared to v6 πŸ‘
With that being said, I do agree that a doc section / page about how to test the components is something we've been talking about for quite some time and that we should definitively do!

We could consider the input a complex widget. I would expect some designers to complain that it's hard to quickly move between fields in a complex form if a date picker takes 4 tabs to move to the next one vs. 1 like the Autocomplete. So maybe to consider a prop to be able to customize this behavior.

That's something we discussed in a meeting a few weeks ago and the team agrees that both approaches can make sense. Do we have some MUI components where the Tab behavior is configurable? So that I can look at there DX. Making it configurable would be nice, and should not be that big of a deal.

Will previous customization break? I guess not. Will it be simpler to customize the input? I guess it will since we have access to more DOM nodes to apply styles.

Customization is the main drawback from this approach.
From what we saw, it will break 3 types of customization:

  1. People who created wrapper around TextField and are using it everywhere it there app (they were able to pass it to slotProps.textField but now the will not be able to do it).
  2. People who customized the TextField in the theme (it won't customize the picker anymore=
  3. People who customized DOM nodes of the TextField that will no longer exist (so the HTML input element).

We still have to work on how to customize the new DOM structure, migrate the doc example and provide new ones.
Concerning the new styling capacity, you are right, people will be able to do new things with the moth detailed DOM structure.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 13, 2023

Tests. People should still be able to fire an onChange event on the input element.

Even with end-to-end tests, e.g. using Playwright?

Tabs. Do we have some MUI components where the Tab behavior is configurable?

I'm aware of this one: https://mui.com/material-ui/react-tabs/#keyboard-navigation that's a bit related, but otherwise no.

Customization. it will break 3 types of customization

Would the approach in https://mui.com/material-ui/react-select/#customization be able to solve this problem? With the Select the React structure looks like this:

<Select
  <Input
     <input slot
       <Complex select child, including Menu, etc.

We have a plan to create a new SelectField using the same structure:

<SelectField
  <TextField
     <input slot
       <Complex select child, including Menu, etc.

I imagine that this structure could be applied to our case.


Other ideas:

@flaviendelangle
Copy link
Member Author

Even with end-to-end tests, e.g. using Playwright?

No indeed, for e2e it will most-likely impact how people are testing it.
cc @LukasTy

Would the approach in https://mui.com/material-ui/react-select/#customization be able to solve this problem? With the Select the React structure looks like this:

Our use case seems quite similar.
What we are trying to customize it the Input, we re-use all the other pieces of the TextField (adornments, label, form control etc...).
cc @noraleonte

How does the form integration look like?

We have a visually hidden input so I think everything should be fine.
Even better than in v6 because we could make this input value "clean" without any UTF-8 hidden char that we have in v6 for edition purpose.

Do we give up on input autocomplete like bday https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete?

Not sure to understand this one.
Was it working in v6? We never discussed it.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 14, 2023
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Nov 22, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 6, 2023
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Terrific work on this one! πŸ’― πŸš€
The other mentioned topics can be addressed in follow ups. πŸ‘Œ

@@ -93,7 +93,7 @@
"@babel/plugin-transform-react-constant-elements": "^7.23.3",
"@babel/preset-typescript": "^7.23.3",
"@mui-internal/docs-utils": "^1.0.0",
"@mui-internal/typescript-to-proptypes": "^1.0.2",
"@mui-internal/typescript-to-proptypes": "https://pkg.csb.dev/mui/material-ui/commit/fd499021/@mui-internal/typescript-to-proptypes",
Copy link
Member

Choose a reason for hiding this comment

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

Friendly reminder to merge and release the relevant package before merging this PR. πŸ˜‰

BaseFieldProps<TValue, TDate, TSection, TEnableAccessibleFieldDOMStructure, TError>,
'unstableFieldRef'
> {
sx?: SxProps<any>;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Shouldn't this be typed with Theme as all the other instances? πŸ€”

Suggested change
sx?: SxProps<any>;
sx?: SxProps<Theme>;

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do, the example with Joy are complaining because the theme of Joy is not the same πŸ‘

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that is an interesting problem. πŸ™ˆ
Maybe @siriwatknp has any idea for an intermediate solution? πŸ€”
It seems that the SxProps from Joy is correctly typed with Joy Theme.
So, technically, this can be an annoying shortcoming. 🀷

Copy link
Member Author

Choose a reason for hiding this comment

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

To summarize :

We have the field component which receives some props and passes them to its textfield.
We have the picker component which renders the fields with some props, including sx (when doing <DatePicker sx={{}} />, the sx prop is forwarded to the field)
We have an interface that describes what the field receives from the picker, to help people building custom field without forgetting some callbacks / some ref / etc... this interface includes the sx prop since the pickers passes it to the field.

For now, we have a single DatePicker component which can be customized through a slot to accept a field built with the Joy UI textfield.
So the interface can use sx?: SxProps<Theme> because Theme is the material theme and not compatible when the field is built for Joy.

The current customization is clearly not optimal and for advanced customization like a Joy UI textfield, it would be better to expose a DatePicker built with Joy UI.

> extends BaseFieldProps<TValue, TDate, TSection, TError> {
interface BaseForwardedCommonSingleInputFieldProps extends ExportedUseClearableFieldProps {
ref?: React.Ref<HTMLDivElement>;
sx?: SxProps<any>;
Copy link
Member

Choose a reason for hiding this comment

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

Same question regarding typing of SxProps.
Shouldn't it be Theme here as well?

packages/x-date-pickers/src/playwright/index.ts Outdated Show resolved Hide resolved
@LukasTy
Copy link
Member

LukasTy commented Feb 14, 2024

Could we also apply this diff (and necessary TS additions) to at least make the native required behavior work? πŸ€”

diff --git a/packages/x-date-pickers/src/PickersTextField/PickersInputBase/PickersInputBase.tsx b/packages/x-date-pickers/src/PickersTextField/PickersInputBase/PickersInputBase.tsx
index e2eb22a23..c34f145de 100644
--- a/packages/x-date-pickers/src/PickersTextField/PickersInputBase/PickersInputBase.tsx
+++ b/packages/x-date-pickers/src/PickersTextField/PickersInputBase/PickersInputBase.tsx
@@ -195,6 +195,7 @@ const PickersInputBase = React.forwardRef(function PickersInputBase(
     inputProps,
     inputRef,
     sectionListRef,
+    required,
     ...other
   } = props;
 
@@ -301,6 +302,7 @@ const PickersInputBase = React.forwardRef(function PickersInputBase(
         id={id}
         aria-hidden="true"
         tabIndex={-1}
+        required={required}
         {...inputProps}
         ref={handleInputRef}
       />
diff --git a/packages/x-date-pickers/src/PickersTextField/PickersTextField.tsx b/packages/x-date-pickers/src/PickersTextField/PickersTextField.tsx
index fc2194ab8..272fe5e84 100644
--- a/packages/x-date-pickers/src/PickersTextField/PickersTextField.tsx
+++ b/packages/x-date-pickers/src/PickersTextField/PickersTextField.tsx
@@ -154,6 +154,7 @@ const PickersTextField = React.forwardRef(function PickersTextField(
         sectionListRef={sectionListRef}
         label={label}
         name={name}
+        required={required}
         {...InputProps}
       />
       {helperText && (

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Feb 14, 2024

@LukasTy done, but I got required from muiFormControl since FormControl already has it.
That way I don't have to add a prop to PickerInputBase

Btw, should we also forward disable to the input?

@LukasTy
Copy link
Member

LukasTy commented Feb 14, 2024

done, but I got required from muiFormControl since FormControl already has it. That way I don't have to add a prop to PickerInputBase

Nice, that's even better. πŸ‘Œ

Btw, should we also forward disable to the input?

Yup, could make sense, it won't hurt.
If we are on it, also applying readonly might make sense. πŸ€”

@flaviendelangle
Copy link
Member Author

disabled and readOnly added

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 19, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Feb 19, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2024
@flaviendelangle
Copy link
Member Author

@LukasTy the prop-types are fixed πŸ‘Œ

@flaviendelangle flaviendelangle merged commit 58f1859 into mui:next Feb 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: pickers This is the name of the generic UI component, not the React module! v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC][fields] HTML structure of the input [fields] Refactor to use multiple elements to support a11y
6 participants