vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
465 stars 83 forks source link

[radio-button] Add readonly state #822

Open rolfsmeds opened 4 years ago

rolfsmeds commented 4 years ago

Description

vaadin-radio-button does not have a readonly state.

Expected outcome

Readonly state should be supported to align with other input field components.

web-padawan commented 2 years ago

According to WHATWG HTML standard: https://html.spec.whatwg.org/multipage/input.html#the-readonly-attribute

Only text controls can be made read-only, since for other controls (such as checkboxes and buttons) there is no useful distinction between being read-only and being disabled, so the readonly attribute does not apply.

rolfsmeds commented 2 years ago

Indeed, the standard html radio-button and checkbox don't have readonly states, and it's a bit tricky to come up with a reasonable readonly representation for them, as I've mentioned for vaadin-checkbox, but I do think the corresponding Vaadin components could still have one, if we can decide on the representation.

dirkey commented 2 years ago

I don't see any technical problem adding a readonly attribute to the vaadin-radio-button. Adding it to the input field itself would be a violation. On semantical level, a readonly property would be a benefit (barrier free etc.). It would make the vaadin-radio-button much more semantical correct. Not to mention the much easier support to render the component. I think it's much worst, if each developer finds it own solution to this quite common problem.

jouni commented 2 years ago

Not sure if this is helpful: if you would be working with the native <input type="checkbox">, how would you solve this issue? Would you add both the disabled and readonly attributes to it, one to disabled interaction and the other for styling?

The reason I’m asking is because I’m looking into a (hypothetical) future where we don't have a separate <vaadin-checkbox> component, but rather rely on the native control instead.

I don't remember how disabled inputs are handled in form submissions, do their values get sent to the server as well?

dirkey commented 2 years ago

I didn't get it with the statements about the underlying <input> control? My comment is about the web component called vaadin-checkbox or vaadin-radio-button. It will be useful, if these components gets a better semantic use. These web components render the requested appearance however they want. If there are using basic HTML tags or doing something crazy via SVG ;-). Its part of the web components to render a readonly attribute to the input tag or not. They need to establish the requested behaviour (best case). Disabled input fields are not included in a form submit. But the basic form submit is not used in modern applications anyway.

jouni commented 2 years ago

Yeah, I understand that with the web components, there’s no technical limitation to not support the read-only state. I was only trying to see this issue from the point of view of native controls: if there ever comes a day when we’d retire <vaadin-checkbox>, how would we handle this use case then?

Form submission was perhaps just for my own curiosity, as web components could be expected to work in that fundamental use case as well, even if in the context of Vaadin it would not be required.

dirkey commented 2 years ago

I think we should abstract this in a readonly state. It makes the most sense. If you think about the input type"radio" or input type='checkbox' the disable attribute is mostly used to emulate this behaviour (for the user). Disable a control is not the common use case for a frontend developer. So, if we implement a checkbox or radio button from ground up, it will be better to implement a readonly attribute. I cannot think about any use case, which need a disable state on the control. Set the disable state on an input is more or less an implementation detail of the component (because there is no other way with the default one). The problem arise from an older idea of the html forms. The textfield got a readonly attribute (and some others). The checkbox and the radio button was primarily implemented as a button not as a field (therefore disable). I think this was a big mistake. At least for a frontend developer it is not used as a button.

dirkey commented 2 years ago

Let me also mention one other benefit: In the current implementation of the flow component RadioButtonGroup you find a method which renders the radio buttons. If it renders readonly, there is a special if statement which renders the selected button differently (as editable, it isn't because all other are disabled ;-)). If you support a readonly attribute on the radio button itself, this special case is not necessary anymore. This makes it much more modular. If you change the underlying rendering of the radio buttons, there is no need to change the implementation of the RadioButtonGroup.

web-padawan commented 2 years ago

Just to clarify: there is a logic for setting disabled state on individual radio buttons in the web component. Even if we decide to add readonly support to vaadin-radio-button, this logic should be preserved.

https://github.com/vaadin/web-components/blob/a0666b5954086ce7400864435380dac4ab1cbc56/packages/radio-group/src/vaadin-radio-group.js#L469-L472

dirkey commented 2 years ago

No, if you add a readonly Property in RadioButton itself, the logic could move into the RadioButton itself. Where it belongs. You need to take care of the checked and readonly property, which renders the RadioButton in the corresponding way. Quite easy and no "logic" dependency between both controls. If you like, I could implement it in my fork. It might even be better to implement this behaviour directly into the web component. Because this component is the nearest part to rendering this. And it is just a rendering issue with the input. If this control is changing from input to something else, it only has to fulfill this task.

dirkey commented 2 years ago

I get quite another concern. If this logic is already written in javascript, why is the same thing repeated in the RadioButtonGroup class in flow component? Is this a mistake? https://github.com/vaadin/flow-components/blob/c754b773a654c5cde12d2deaa39f04e661bf696f/vaadin-radio-button-flow-parent/vaadin-radio-button-flow/src/main/java/com/vaadin/flow/component/radiobutton/RadioButtonGroup.java#L691-L707

knoobie commented 2 years ago

Security. Web-components are client-side implementations and can't be trusted.

dirkey commented 2 years ago

I am not quite sure. What are the security issues in a readonly attribute? Disable state could be faked as well. I see security issues for web components if they load code or invoke urls from other sides.