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

feat: add native input DOM to follow standard behavior of HTML #1018

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nnmax
Copy link

@nnmax nnmax commented Jan 5, 2024

Related

#790

ant-design/ant-design#36489

💡 Background and solution

<Select /> is a form element which should have the ability to interact with the form.

Ant Design is a great project that has brought great convenience to front-end developers all over the world.

However, until now, Ant Design's <Select /> still doesn't support this functionality. We had to use some hacks to implement it:

const App = () => {
  const defaultValue = "lucy";
  const [name, setName] = useState(defaultValue);

  const handleChange = (changedValue: string) => {
    setName(changedValue);
  };

  const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {
    event.preventDefault();
    const formData = new FormData(event.currentTarget);
    console.log("name", formData.get("name"));
  };

  return (
    <form onSubmit={handleSubmit}>
      <Select
        defaultValue={defaultValue}
        style={{ width: 120 }}
        onChange={handleChange}
        options={[
          { value: "jack", label: "Jack" },
          { value: "lucy", label: "Lucy" },
          { value: "max", label: "Max" },
        ]}
      />
      <input name="name" type="hidden" value={name} />
      <button type="submit">Submit</button>
    </form>
  );
};

This PR implements the above functionality and simply passes in nativeInputProps to do so:

const App = () => {
  const defaultValue = "lucy";
- const [name, setName] = useState(defaultValue);

  const handleChange = (changedValue: string) => {
    setName(changedValue);
  };

  const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {
    event.preventDefault();
    const formData = new FormData(event.currentTarget);
    console.log("name", formData.get("name"));
  };

  return (
    <form onSubmit={handleSubmit}>
      <Select
        defaultValue={defaultValue}
        style={{ width: 120 }}
        onChange={handleChange}
+       nativeInputProps={{ name: 'name' }}
        options={[
          { value: "jack", label: "Jack" },
          { value: "lucy", label: "Lucy" },
          { value: "max", label: "Max" },
        ]}
      />
-     <input name="name" type="hidden" value={name} />
      <button type="submit">Submit</button>
    </form>
  );
};

The above code renders a hidden input DOM to receive form related attributes. In case of <Select /> with multiple or tags mode, the value of the input is a comma-separated string.

The type of nativeInputProps is React.InputHTMLAttributes<HTMLInputElement>, which accepts all input props.

Copy link

vercel bot commented Jan 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
select ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2024 6:10am

@nnmax
Copy link
Author

nnmax commented Jan 5, 2024

If there are any problems, I am willing to actively update this PR until it is merged.

expect(container.querySelector('input.rc-select-native-input').getAttribute('id')).toEqual('test');
expect(container.querySelector('input.rc-select-native-input').getAttribute('name')).toEqual('test');
expect(container.querySelector('input.rc-select-native-input').getAttribute('value')).toEqual('test 2');
})

Check notice

Code scanning / CodeQL

Semicolon insertion Note test

Avoid automated semicolon insertion (97% of all statements in
the enclosing function
have an explicit semicolon).
expect(container.querySelector('input.rc-select-native-input').getAttribute('id')).toEqual('test');
expect(container.querySelector('input.rc-select-native-input').getAttribute('name')).toEqual('test');
expect(container.querySelector('input.rc-select-native-input').getAttribute('value')).toEqual('test 1,test 2');
})

Check notice

Code scanning / CodeQL

Semicolon insertion Note test

Avoid automated semicolon insertion (97% of all statements in
the enclosing function
have an explicit semicolon).
expect(container.querySelector('input.rc-select-native-input').getAttribute('id')).toEqual('test');
expect(container.querySelector('input.rc-select-native-input').getAttribute('name')).toEqual('test');
expect(container.querySelector('input.rc-select-native-input').getAttribute('value')).toEqual('test 1,test 2');
})

Check notice

Code scanning / CodeQL

Semicolon insertion Note test

Avoid automated semicolon insertion (97% of all statements in
the enclosing function
have an explicit semicolon).
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c93ff10) 99.78% compared to head (0a8c1d7) 99.78%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1018   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files          38       39    +1     
  Lines        1398     1405    +7     
  Branches      391      391           
=======================================
+ Hits         1395     1402    +7     
  Misses          3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -106,6 +108,12 @@ const SingleSelector: React.FC<SelectorProps> = (props) => {
/>
</span>

<SelectNativeInput
Copy link
Member

Choose a reason for hiding this comment

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

There exist a input for search usage. Seems that one should be aria hidden?

Copy link
Author

Choose a reason for hiding this comment

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

The role of the search input is a combobox, which contains many aria-* attributes that should not be aria hidden.

<SelectNativeInput /> is already aria hidden.

Copy link
Member

Choose a reason for hiding this comment

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

Why make SelectNativeInput aria hidden?

Copy link
Author

Choose a reason for hiding this comment

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

I've changed the code:

export default React.forwardRef<HTMLInputElement, SelectNativeInputProps>(
  function SelectNativeInput(props, ref) {
    const { prefixCls, className, ...rest } = props;

    return (
      <input
-       aria-hidden
-       tabIndex={-1}
        type="hidden"
-       readOnly
        ref={ref}
        className={classNames(`${prefixCls}-native-input`, className)}
        {...rest}
      />
    );
  },
);

Thank you. You're right. aria-hidden shouldn't be set. But type="hidden" was already set. The screen reader still can't read it.

Should we remove the type='hidden' and add some accessibility attributes to make it accessible to screen readers?

Copy link
Member

Choose a reason for hiding this comment

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

I guess could use opacity: 0 + 0 width & height to let it invisible on the screen but still workable with reader.

Copy link
Author

Choose a reason for hiding this comment

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

done

import classNames from 'classnames';
import React from 'react';

+ const visuallyHidden: React.CSSProperties = {
+   position: 'absolute',
+   overflow: 'hidden',
+   width: 1,
+   height: 1,
+   border: 0,
+   margin: -1,
+   padding: 0,
+   clip: 'rect(0 0 0 0)',
+   whiteSpace: 'nowrap',
+ };

interface SelectNativeInputProps extends React.InputHTMLAttributes<HTMLInputElement> {
  prefixCls?: string;
}

export default React.forwardRef<HTMLInputElement, SelectNativeInputProps>(
  function SelectNativeInput(props, ref) {
    const { prefixCls, className, style, ...rest } = props;

    return (
      <input
-       type="hidden"
        ref={ref}
        className={classNames(`${prefixCls}-native-input`, className)}
+       style={{ ...visuallyHidden, ...style }}
        {...rest}
      />
    );
  },
);

@zombieJ
Copy link
Member

zombieJ commented Jan 22, 2024

Since Select always has the search input, seems it will confuse the reader to check the real value holder if search input exist. Could it to add aria value on the search input instead to patch another input?

@nnmax
Copy link
Author

nnmax commented Jan 22, 2024

Since Select always has the search input, seems it will confuse the reader to check the real value holder if search input exist. Could it to add aria value on the search input instead to patch another input?

What aria value should I add to the search input?

@KMJ-007
Copy link

KMJ-007 commented Jun 21, 2024

any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants