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

[text-field] Invalid readonly text field has too bright background color #1196

Closed
Haprog opened this issue Jan 23, 2020 · 12 comments · Fixed by #7204
Closed

[text-field] Invalid readonly text field has too bright background color #1196

Haprog opened this issue Jan 23, 2020 · 12 comments · Fixed by #7204
Assignees
Labels
bug Something isn't working Impact: Low Severity: Minor vaadin-text-field workaround There is a workaround in the comments.

Comments

@Haprog
Copy link
Contributor

Haprog commented Jan 23, 2020

Description

<vaadin-text-field> which is both readonly and invalid has incorrect styles. The background color is too bright red. Might also affect other fields in this repo (should be checked).

The cause of the problem is that the ::after pseudo-element of part="input-field" is used for overlaying a slight color change on hover (when the field is not readonly) but the same element is also used for showing the dashed border when the field is readonly. The dashed border should have opacity: 1 but the background color overlay should have opacity: 0.1 so this is the conflict.

Expected outcome

<vaadin-text-field> which is both readonly and invalid should have the same kind of background as a normal invalid text field (also similar to the combination of disabled and invalid).

It should look something like this:
image
(this screenshot was made by moving the dashed border from ::after element directly to the part="input-field" but that has the undesired side effect of making the input 1px larger in every direction by adding a border)

Actual outcome

Actually it looks like this:
image

Steps to reproduce

  1. Go to https://cdn.vaadin.com/vaadin-text-field/2.5.3/demo/#text-field-basic-demos
  2. Inspect the first <vaadin-text-field> and give it the attributes readonly and invalid

Browsers Affected

All.

@vaadin-bot vaadin-bot transferred this issue from vaadin/vaadin-text-field May 19, 2021
@web-padawan web-padawan changed the title Invalid readonly text field has too bright background color [text-field] Invalid readonly text field has too bright background color May 28, 2021
@web-padawan
Copy link
Member

The same fix should be applied to vaadin-text-area, see vaadin/flow-components#2010

@jouni
Copy link
Member

jouni commented Aug 18, 2021

I'm curious to hear the case where this becomes an issue.

I assume the case is something where an invalid value has somehow been introduced to the data. It might have been valid before, but some other conditions in the app have changed (for example, a date has passed), and the non-editable presentation of the data is handled with a read-only field (which is not necessarily ideal). The invalid state is used as a prompt to encourage the user to fix the data.

Some could say this is a rare case, and should be avoided in the first place (“if I can’t edit the field, why show me it’s invalid?”). But if it’s possible to fix, why not? But it might be a breaking change, because of the CSS implementation. I hope we can simplify that at some point in the future.

@mrgreywater
Copy link

mrgreywater commented Aug 18, 2021

For my usecase, I wanted to display a TextArea with an error message accompanied with a suffix button-component to copy the string to a clipboard. I didn't want to bother manually creating a new Component when an existing Component (TextArea) already had all necessary parts.
I set it to readonly to make the error message non user editable, and set to invalid for the red background of the error message.

@jouni
Copy link
Member

jouni commented Aug 18, 2021

If I understood that correctly, you would’ve probably wanted to use something like a “error message" component (similar to https://atlassian.design/components/section-message/examples#error), which would have a built-in "copy to clipboard" button.

Using a read only Text Area for this purpose is a clever workaround, but not something I would recommend, mostly because of accessibility. It might confuse screen reader users.

@mrgreywater
Copy link

@jouni Personally I don't think it's relevant that there are cleaner ways to do the same task. Sure, I can spend a few hours designing a better custom error component, but if I do that, I also have to keep it updated with the sizing changes of vaadin in the future. Using an existing TextArea makes sure that it will have all the correct padding, margin to fit in with all the other elements now and in the future.
Even if I find an existing vaadin addon, they are often not updated in a timely fashion, so I have to fork it and update it myself whenever there are breaking Vaadin changes (which happens relatively often).

Also the current behaviour is buggy either anyway, it may not be of high importance, but a bug regardless.

@jouni
Copy link
Member

jouni commented Aug 18, 2021

Yeah, I can see the trouble with keeping up with changes in the built-in components. My point was actually that maybe Vaadin should offer such a component out of the box.

@knoobie
Copy link
Contributor

knoobie commented Feb 3, 2022

I stumbled upon this while migrating; isn't the fix "kinda easy" now? In an isolated test page, this worked as a fix. (Edit:In a complex page this looks good as well)

Faulty:

:host([invalid])::after {
  background-color: var(--lumo-error-color-50pct);
}

Looking good:

:host([invalid]:not([readonly]))::after {
  background-color: var(--lumo-error-color-50pct);
}

@jouni
Copy link
Member

jouni commented Feb 8, 2022

I suppose that would be good workaround. A readonly field doesn't show the hover or "activation" styles anyway, so it doesn't matter if the color is not adjusted in this case.

I suppose it’s a different issue, that an invalid readonly field loses the invalid state if you focus and blur the field?

@web-padawan
Copy link
Member

I suppose it’s a different issue, that an invalid readonly field loses the invalid state if you focus and blur the field?

Yes, it's a different issue. Here is one for date-picker: #1750

@knoobie
Copy link
Contributor

knoobie commented Feb 8, 2022

I'm currently fixing this issue in my application with the following css inside the vaadin-input-container.css (I hope that this component is public styling api)

/* fix wrong red for invalid readonly fields https://github.com/vaadin/web-components/issues/1196 */
:host([invalid])::after {
  background-color: initial;
}

:host([invalid][theme~='error']:not([readonly]))::after {
  background-color: var(--lumo-error-color-50pct);
}

(Note: I have more themes (not just error), that's why I've added the theme there as well)

Edit: Wanted to mention it; it isn't so severe - with an easy fix / workaround.

@web-padawan web-padawan added the workaround There is a workaround in the comments. label Feb 8, 2022
@knoobie
Copy link
Contributor

knoobie commented Mar 13, 2024

Cheeky push; kinda annoying to explain that this additional css is mandatory for a basic example like a read-only text with validations errors which implies to the user: press edit button to fix it

@web-padawan
Copy link
Member

Sorry about the issue left without attention. I created a PR that adds styles from your workaround: #7204

@web-padawan web-padawan self-assigned this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Impact: Low Severity: Minor vaadin-text-field workaround There is a workaround in the comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants