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

Duplicate clear icon in (date) input #791

Closed
oliversalzburg opened this issue Jun 18, 2022 · 5 comments · Fixed by #794
Closed

Duplicate clear icon in (date) input #791

oliversalzburg opened this issue Jun 18, 2022 · 5 comments · Fixed by #794
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@oliversalzburg
Copy link
Contributor

oliversalzburg commented Jun 18, 2022

Describe the bug

Under certain conditions, the sl-input will show two clear icons.

To Reproduce

Unclear so far.

Demo

https://codepen.io/oliversalzburg/pen/vYdMWOe?editors=1000

Screenshots

image

Browser / OS

  • OS: WIndows
  • Browser: Firefox
  • Browser version: 101.0.1
@oliversalzburg oliversalzburg added the bug Things that aren't working right in the library. label Jun 18, 2022
@claviska claviska added the good first issue This bug or task is a good first issue for new contributors. label Jun 18, 2022
@bunesk
Copy link
Contributor

bunesk commented Jun 18, 2022

I can only reproduce this in Firefox. It seems to me that Firefox itself inserts this clear button because if you remove the clearable you still have one clear icon and it is directly within the input element. It also appears in a regular <input type="date">.

@oliversalzburg
Copy link
Contributor Author

You're right. This only happens in Firefox. Not sure what I was thinking yesterday.

@oliversalzburg
Copy link
Contributor Author

So for Webkit-based browsers, there's a -webkit-clear-button pseudo element. This pseudo element isn't supported in Firefox. There's a 4 year old issue on the subject and no alternative approach was provided there.

I've also seen this issue brought up in another component library, where they ended up implementing the entire picker themselves (not only because of this issue though).

Other common approaches seem like hacks that I wouldn't want to use for this this: https://stackoverflow.com/questions/49527186/how-to-remove-cross-on-date-and-time-html-inputs-in-firefox

Should Shoelace's clear button just always be hidden on Firefox then?

@bunesk
Copy link
Contributor

bunesk commented Jun 19, 2022

Yeah I also didn't find something else to solve this. But in my opinion it is still better to use clip-path to solve this like this:

input[type='date'] {
  clip-path: inset(0 2em 0 0);
}

I think this would be better than hiding the shoelace clear button because the clear button of Firefox:

  1. shows also up when clearable is not set
  2. always has the same size, no matter which size you set to the sl-input
  3. the position of the clear icon is also not that good

What are you thinking about this, @claviska? If you both agree I can write a PR for that.

@claviska claviska removed the good first issue This bug or task is a good first issue for new contributors. label Jun 20, 2022
@claviska
Copy link
Member

This was a tough decision because there were zero browser sniffs in the library before 5f25049. After extensively researching ways around this, it appears as though @Buni48's clip-path solution is the most practical one. However, it causes adverse effects in other browsers, hence the sniff.

However, I'm OK with the user agent sniff (and prefer it over feature detection of unrelated properties since those are likely to change over time) because, if it fails, the clear icon will simply become redundant. This fix should work perfectly for 99.9% of users, and anyone changing their UA string will simply see an extra icon but they won't lose any functionality.

Until there's a better solution, e.g. a -moz pseudo element or a more reasonable workaround, this seems like the best path forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants