fix(SearchInput/TextInputGroup): remove browser clear icons

[thatblindgeye]

closes #5210

I added the rules to both files because Core uses pf-c-search-input classes for the Search Input, but React uses Text Input Group under the hood (not sure if there's an issue open about making this more consistent, I didn't come across one).

This How to Remove “X” icon from search input field or input type search article is what I came across.

To test (use Chromium or Safari):

1. Go to search input preview build
2. Update the first example so the input has type="search"
3. Enter text in the input and notice how an icon does not get rendered by the browser
4. Do steps 2-3 on the HTML search input on the live site, and notice how an icon does get rendered

[patternfly-build]

Preview: https://patternfly-pr-5211.surge.sh

A11y report: https://patternfly-pr-5211-a11y.surge.sh

[mcoker]

This looks great. I opened an issue to replace our use of the search input with the text input group. #5212

On this, we show examples of search inputs without without our clear button (a use case may be when there is not enough space to show it) - in those cases, would we still want to remove the browser generated clear button? Even if it's inconsistent, I know some people use particular browsers for their UI with form elements like this.

Related, what about the form control (<TextInput>)? Seems like the same use case, except the text input and search input components take an onClear event that will create the clear button, and the form control doesn't - it would need to be manually paired with a clear button in an input group - https://github.com/patternfly/patternfly/blob/main/src/patternfly/components/FormControl/examples/FormControl.md#icon-sprite

[thatblindgeye]

On this, we show examples of search inputs without without our clear button (a use case may be when there is not enough space to show it) - in those cases, would we still want to remove the browser generated clear button? Even if it's inconsistent, I know some people use particular browsers for their UI with form elements like this.

Yeah it's tricky for when the clear button provided by PatternFly may need to be omitted due to spacing. Some possiblities:

• Remove the browser clear button + adjust the styles for text input group so that at smaller widths, everything renders correctly (right now pf-c-text-input-group at around ~175px width seems to be about as small as you can go before the clear button for SearchInput starts creeping visually into the input (image as at 150px width):
  
• Have these rules as part of a class that can be applied by the consumer. This could then be added in React only when the search input's clear button is rendered, though in Core it would more be up to the consumer to ensure the dual clear button behavior doesn't occur
• Only have the browser rendered button removed completely (what this PR currently does)
• Other (if there are other ideas out there)

At a minimum I think it'd be ideal to at least prevent the browser clear + our clear from both rendering (which option 2 would help resolve). Personally I would opt to also prevent the browser one from rendering and having the TextInputGroup's/SearchInput's used, since it would provide everyone (regardless of browser) a means to immediately clear the input, make things more consistent across browsers, and make input elements more consistent (rather than only some inputs having a browser rendered clear button, and others not having one). That said, it could very easily skirt the line of being something nobody really notices, or a significant change for users.

Related, what about the form control ()? Seems like the same use case, except the text input and search input components take an onClear event that will create the clear button, and the form control doesn't - it would need to be manually paired with a clear button in an input group

Depending what route we'd want to go down, we could also add these styles to pf-c-form-control.

[mcoker]

At a minimum I think it'd be ideal to at least prevent the browser clear + our clear from both rendering (which option 2 would help resolve). Personally I would opt to also prevent the browser one from rendering and having the TextInputGroup's/SearchInput's used, since it would provide everyone (regardless of browser) a means to immediately clear the input, make things more consistent across browsers, and make input elements more consistent (rather than only some inputs having a browser rendered clear button, and others not having one).

@thatblindgeye I agree, #2 seems like a good option. An issue is a simple <TextInput> - I think that's pretty commonly used, and to get a close button, you'd need to use the <TextInputGroup> or something more complex. Personally, I'd be in favor of making the <TextInputGroup> the default way of creating a text input as long as we're adding all of these customizations. It gives us the structure/ability to customize and attach all of this stuff to the text input that you can't do with a simple <input type="text"> element. wdyt @srambach @mattnolting?

[mcoker]

Re: a class, I think it could go a couple of ways.

• A class whose job is to remove the clear icon, eg .pf-m-no-clear-icon.
• A .pf-m-close class on the text input group, assuming that's our suggested way of using a text input to create a clear button (moving away from a simple input group with a <TextInput> + <Button>). This class is a bit more versatile, and could apply the styling to remove the browser's clear button and manage the presence of the PF clear button/layout in other ways that work better for when the text input group has a clear button attached to the input. For example, maybe some layout rework to handle the narrow viewport/overlap bug you mentioned above. My vote would be for this.

Then we could maybe add a class to the <TextInput> that removes the browser icon when either a user has used a simple <InputGroup> to create a clear button, or for some other use.

[srambach]

If I read it correctly, then this is removing the browser clear (which appears in safari/chrome but not in FF) in favor of manually adding a clear button? In this particular case, I'd favor consistency within a browser over consistency across browsers. Altering default browser behavior can be risky with no guarantee of future behavior.

[mattnolting]

In testing w/VO, I'm not seeing a huge benefit to updating type to search, especially bc the field is described by an aria-label. It seems most reasonable, given all the extras provided by the affected components (close button, result count, etc), to use only type="text" and update the aria-label to something like Search by name. Best to avoid, wherever possible, changing specific browser default functionality, as future updates create unpredictable results. @mcoker @srambach wdyt?

[thatblindgeye]

If I read it correctly, then this is removing the browser clear (which appears in safari/chrome but not in FF) in favor of manually adding a clear button? In this particular case, I'd favor consistency within a browser over consistency across browsers. Altering default browser behavior can be risky with no guarantee of future behavior.

@srambach That's a really good point! Maybe instead of tinkering with any of this, we can just recommend having our clear button rendered when necessary via examples and/or documentation?

In testing w/VO, I'm not seeing a huge benefit to updating type to search, especially bc the field is described by an aria-label. It seems most reasonable, given all the extras provided by the affected components (close button, result count, etc), to use only type="text" and update the aria-label to something like Search by name. Best to avoid, wherever possible, changing specific browser default functionality, as future updates create unpredictable results.

@mattnolting Yeah with VO the only difference I think is "search text field" being announced when type="search" vs "edit text" (NVDA and JAWS for me I didn't notice any difference). aria-labels could help differentiate, and the React examples do use "Search input" as the default aria-label. Based on JAWS and NVDA, at least for me, not signifying the type of input like VO does, I think the only pros I can think of using type="search" is 1) it's slightly more accurate (though in 2/3 of screen readers that accuracy doesn't seem to do much), and 2) it may replace the typical "Enter" symbol on touch keyboards with a magnifying glass.

There have already been some updates in React that update instances of Search Input to use type="search", so depending where we'd land those may have to be reverted.

[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]

Closing per discussion in the linked issue.