Bug - Inputs: inconsistencies across browsers for search types

[thatblindgeye]

Describe the problem
When using an <input type="search" /> (such as is being done in some React PRs, see patternfly/patternfly-react#8040 and patternfly/patternfly-react#8293, the latter reverted the type for now though), the input gets rendered differently.

In Chromium/Safari, an "x" icon will render inside the input, which can be clicked to clear it. In Firefox, however, no such icon gets rendered.

Rather than just using type="text" for search inputs, we should use type="search" since it'd be more semantically correct, and depending on the assistive tech it would get announced as a search text input.

A followup to this would also involve using the correct type for inputs/allowing the correct types to be used. Some notes:

• The dual list selector PR above added a type prop to the SearchInput component, which it may be better to only allow type="search" since we have other input components to pass a type into.
• The TextInput and TextInputGroupMain components in React defaults to the "text" type, but also allows a "search" type
  • For TextInputGroupMain, type="text" is hardcoded in a native input when the hint prop is passed
• Core and React have inconsistency in the markup of the SearchInput component. Core uses pf-c-search-input classes, while React uses the TextinputGroupMain under the hood

The following list shows currently where an input is being used, what type is being used, and whether a native input or one of our component's is being used. Most of this pertains to React implementation, but should be relatively 1:1 with Core (Core also has several checkbox inputs in Table demos)

Input uses cases in PatternFly

• Currently using type="text":
  • Label (for editable variant; native input element)
  • AdvancedSearchMenu (TextInput component; passes "text" into the type prop)
  • TextArea (only in examples; the type could be omitted)
  • TimePicker (TextInput component; passes "text" into the type prop)
  • PasswordStrength demo (TextInput component)
  • DateTimePicker demo (TextInput component; passes "text" into the type prop)
  • all 3 HelperText demos (TextInput for all 3; passes "text" into the type prop)
  • TableTextInput example (TableTextInput component; inline-edit extension)
  • FileUploadDemo (FileUpload component; for the demo-app in react-integration)
  • EditableTextCell (TextInput component; in react-table)
  • ViewToolbar (TextInput; in react-topology)
  • ClipboardCopy (uses TextInput's default of "text")
  • DatePicker (uses TextInput's default of "text")
  • FileUploadField (uses TextInput's default of "text")
  • TextInput examples (passes in "text" to type prop)
  • PasswordGenerator demot (TextInput; uses the default for the type prop)
  • TextInputGroupMain examples (the "disabled" example passes "text" into the type prop, the other examples do not; Core only uses "text" in examples)
  • ComposableTypeaheadSelect and ComposableMultiTypeaheadSelect (TextInputGroupMain; uses the default type prop of "text")
  • AttributeFiltering and AutoCompleteSearch demos for TextinputGroup (TextInputGroupMain; uses the default type prop of "text")
  • SelectMultiTypeahead and SelectTypeahead examples for Select Next (TextInputGroupMain; uses the default type prop of "text")
  • SearchInput (TextInputGroupMain; uses default type prop of "text", but can be overridden; Core only uses "text" in examples)
• Currently using type="search":
  • ApplicationLauncher (native input element; the above linked PR would change this)
  • ContextSelector (TextInput component)
  • DualListSelector (SearchInput element, see PR linked above)
  • TreeViewSearch (native input element)
  • PrimaryDetail (TextInput component, multiple times)
  • ComposableApplicationLauncher demo (TextInput component)
  • ComposableContextSelector demo (TextInput component)
  • MenuDemo (TextInput component; for the demo-app in react-integration)
  • ToolbarDemo (TextInput component; for the demo-app in react-integration)
• Currently using type="number":
  • CalendarMonth (TextInput component)
  • NumberInput (TextInput component)
  • Pagination > Navigation (native input element)
  • Slider (TextInput component)
  • FormDemo (FormGroup component; for the demo-app in react-integration)
  • TopologyPackage (TextInput component; in react-topology)
  • Tooltip examples
• Currently using type="email":
  • ModalWithForm example (TextInput component)
• Currently using type="checkbox":
  • Card (native input element)
  • Checkbox (native input element)
  • DataListCheck (native input element)
  • DropdownToggleCheckbox (native input element)
  • DualListSelectorTreeItem (native input element)
  • MenuToggleCheckbox (native input element)
  • SelectOption (native input element)
  • Switch (native input element)
  • TreeViewListItem (native input element)
• Currently using type="radio"
  • DataListItem (native input element)
  • Radio (native input element)
• Currently using multiple types
  • Select (uses "text" and "search" in different parts of the React code; native input element for all)
  • Button demo (uses "password" and "text"; TextInput component for both)
  • InputGroupDemo (uses multiple types, all on Textinput component; for the demo-app in react-integration)
  • SelectColumn (can use either "checkbox" or "radio"; native input element)
  • TextInputGroupMain (can use either "text" or have type passed in; native input element)
  • Form examples (uses multiple type; all using either FormGroup or TextInput components; FormGroup may be using type incorrectly)
  • InputGroup examples (uses multiple types, all using TextInput component)
  • LoginForm (uses either "text" or "password"; TextInput component)

Screenshots
If applicable, add screenshots to help explain the issue.

Chrome (similar in Safari):

Firefox:

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 30 days if no further activity occurs.

[thatblindgeye]

To tl;dr some of the discussion happening in the linked PR:

• consistency within a browser could be favored over consistency across browsers
• there may not be any huge benefit to using type="search" over a default text input.
  • what do you get for using a search input type

Two possible routes to go, rather than removing the browser icons like the linked PR does, could be 1) not use type="search" in our input components (possibly rely on PatternFly rendered clear button instead, as seen in the first search input example), or 2) use type="search" but not use a PatternFly rendered clear button (would rely on the browser clear button if it renders one, otherwise no clear button).

cc @mcarrano @mmenestr for your input

[mmenestr]

Discussed on Slack - but am in favor of removing the browser X and enforcing the PF X everywhere so that we don't run the risk of having to Xs at any point

[thatblindgeye]

@nicolethoen @mattnolting @srambach Based on Margot's input above, would we want to update/revert our Search Input component so that it's a simple type="text' input (using aria and some visual labeling to convey it as a search input), and then go through examples to ensure the Search Input's clear button is rendered when applicable?

Nicole, in react that would involve going through examples/demos and removing any type="search" passed in. And possibly deprecating the type prop since I can't imagine us wanting to allow Search Input type being changed (consumers should be able to use Text Input for that sort of use case)?

[nicolethoen]

sounds like that's what we'd need to do!

[mattnolting]

@thatblindgeye

...would we want to update/revert our Search Input component so that it's a simple type="text' input (using aria and some visual labeling to convey it as a search input), and then go through examples to ensure the Search Input's clear button is rendered when applicable?

Yep, I agree. Considering the built in functionality of search input and limited benefit of type="search", we should update the examples.

[nicolethoen]

We are opting for the react approach of removing type=search from inputs on the react side.

[mcoker]

We're using type="search" in core in these two places:

• patternfly/src/patternfly/components/TextInputGroup/text-input-group--search-input.hbs

  Line 1 in 8ac660d

  {{#> text-input-group text-input-group-text-input--type="search"}}

  • Should we update that to type="text" to match?

• patternfly/src/patternfly/components/FormControl/examples/FormControl.md

  Line 170 in 8ac660d

  {{> form-control controlType="input" input="true" form-control--modifier="pf-m-search pf-m-icon-sprite" form-control--attribute='type="search" value="Search" id="input-search-sprite" name="search-input" aria-label="Search input example"'}}

  • I'm assuming we'll leave this alone since it's just a standalone input and we don't render a close button for it?

[thatblindgeye]

@mcoker

• I'd say yeah, the Core search-input should be updated to be a type="text" since that would be the update in React.
• I feel like that might be fine to leave as is... An alternative would be to remove that input from the example to sort of nudge just using our SearchInput component, though that depends how significant it might be to include.