Add "disable tag removal" setting

[a3kov]

This adds a new component setting which allows to disable removal of currently selected tags. With this setting on one can implement "read only" forms.

Currently LS has "sticky" attribute for options but its purpose is to lock a single option, and I couldn't make it work well with assocs/embeds - it's honestly too much work if all you need is "read only" mode. Besides, this new option prevents removal on the event callback level too.

[maxmarcon]

have you tried setting the value attribute?

[a3kov]

"value" is broken for me, see #103
And from what I understand, "remove" buttons will be rendered AND will work even with the value setting on.
Generally in "read only" mode we want to completely lock down the form to emphasize it's read only state.

[maxmarcon]

Ok I'll look into the other issue in detail when I find some time, hopefully soon.

Let me try to understand the use case here. What do you mean by read-only form? It's a form with a predefined value that the user can't change, but it still gets submitted to the server?

If the answer is yes, then fixing #103 plus using sticky for the options would work.

and I couldn't make it work well with assocs/embeds

can you elaborate on this please? Thanks

[a3kov]

Sure, I'll explain.
"Read only" state can exist both for the component (a single field can't be modified after save) or for the whole form (the form can't be submitted). In the former case we render the component but don't allow to change the selection. In the latter case we hide the form submit button, but still render the form to show the user currently saved values. It's very useful because then you don't need to create a separate view for settings - you simply re-use the form in "read only" mode.
As far as I know both "component-level read only" and "form-level read only" modes are commonly used in software projects. In HTML its supported via "disabled" and "readonly" attributes for inputs.

I could not make "sticky" work well with assocs/embeds - the value_mapper/1 callback you built-in will convert the data into the option, but there's no form state involved, that means the option must be always sticky or always not sticky. If you are suggesting that the embed/assoc must contain "sticky" attribute for the form - that would be very bad design - data layer should not be concerned with UI state.

[maxmarcon]

Thanks for the explanation. I haven't suggested anything yet.

Here's my actual suggestion: to use the disabled attribute in the component. That will render the selection but the user won't be able to interact with it. I just checked and this is unfortunately currently broken in tags mode, as the user can still remove the tags in disabled mode. But that's an easy fix.

If disabled mode is made to work in tags mode, and #103 is fixed, would that be enough to implement your readonly form?

[a3kov]

Yes I am aware of the "disabled" attribute, however you explicitly stated in the docs that it affects only the text input. I think if you suddenly make it also disable tag removal, it may break some user's code that depended on the current behavior. That's why I chose to create a new attribute.

There could be cases where you want to disallow adding new tags, but still allow removing existing tags. I'm not sure about it, but it's a possibility

Regarding #103 - I've stopped using that setting, as I don't need it. It works just fine with the form values OOTB

[maxmarcon]

Yes I am aware of the "disabled" attribute, however you explicitly stated in the docs that it affects only the text input. I think if you suddenly make it also disable tag removal, it may break some user's code that depended on the current behavior.

The docs are wrong/incomplete :) disabled means the user can't interact with the control, so no interaction (including removing tags) should be possible

And this conversation got me thinking that there is a difference between disabled and readonly: disabled means the user can't interact with the input AND the value isn't submitted when the form is. Readonly simply means the user can't interact with the input. I feel it would be nice to provide both, although I just saw that the HTML <select> element doesn't support readonly.

[a3kov]

The docs are wrong/incomplete :)

Are they ?
It's up to you, of course, you are the maintainer. But in my view if it works according to the docs, then it must be intended. I would consider that a breaking change

[maxmarcon]

But in my view if it works according to the docs, then it must be intended. I would consider that a breaking change

No, because tags mode wasn't a thing when disabled was introduced, and neither the behavior nor the docs were updated after tags mode was added.

I made sure that the button to clear the tags and the selection now honor the disabled attribute. Changes are pushed to master. Take a look and let me know, thanks.

[a3kov]

I would also add disabled state check in the event handlers (like in my PR), just to be sure. Not that it's a good idea to trust form data but maybe someone is doing that... wouldn't hurt anyway

[maxmarcon]

You mean raising if someone sends option_removal or clear events in disabled mode?

If someone is doing that than they're explicitly sending them from the front-end, bypassing the component UI. They're "not "supposed" to do that. They're messing with the internal details of the component and so I don't feel like we have to guarantee anything.

[a3kov]

Exactly, it is to prevent forbidden actions by an malicious actor.
Generally you would have additional checks on submit, but if someone forgot to implement those, the built-in guards in the component could help. Also one could use the component without the submit button - if it's used as a dynamic filter for example.

[maxmarcon]

We're talking about 2 events here: option_removal and clear. These are events internal to the component, they have nothing to do with the form data or the form submit or change events.

If the UI of the component isn't sending them, I don't feel like I have to worry about a developer sending them explicitly from their code. If they're doing so, they're meddling with the internal state of the component and there's nothing I can or should do to protect them

[a3kov]

You don't have to worry, sure. It's just the attitude - doing the bare minimum or going above and beyond.
I prefer the second. But it's up to you of course.

[maxmarcon]

You don't have to worry, sure. It's just the attitude - doing the bare minimum or going above and beyond.
I prefer the second. But it's up to you of course.

No, it's about understanding the meaning of clear contracts and interfaces. You don't mess with the internal implementation of a software component. You use the interface. If you want to mess with the internal state, you can do it, but you should know that you're on your own. Trying to add protection or error handling there is both (1) useless and (2) hopelessly defensive, because no amount of protection will be enough

[maxmarcon]

I'm gonna close this MR now, thanks for the fruitful interaction

[a3kov]

No, it's about understanding the meaning of clear contracts and interfaces

What contracts are you talking about if we are talking about malicious actor ? What you are saying doesn't make any sense. Besides, nobody would get hurt if you added those checks. It's not a rational decision

[maxmarcon]

I would invite you to use a more respectful tone. I guarantee you you have only to gain from it.

What contracts are you talking about if we are talking about malicious actor ?

The contract I'm talking about is the fact that these events are internal to the component and are not part of any user interface. If you send them, it means you've read the code and your trying things out, which is perfectly ok but again, you should expect things to break when doing so.

A malicious actor here can only be someone who performed a successful XSS attack on the page, and in this case, all bets are off, no amount of protection (certainly not raising an exception in an event handler) will be enough.

[a3kov]

A malicious actor here can only be someone who performed a successful XSS attack on the page, and in this case, all bets are off, no amount of protection (certainly not raising an exception in an event handler) will be enough.

A malicious actor is a user sending unexpected data in WS messages (remember we always should treat data coming from the client as untrusted). It's not the owner of the website/application or a user who is following contracts.
Like I said, in a situation where the developer missed some checks, the additional strict checks in the component could help. And since these checks don't alter behavior of the component for well-behaving users, I see no downsides in having them. You just decided against it irrationally, but in general having more strict software is a good thing.

[maxmarcon]

(remember we always should treat data coming from the client as untrusted)

I know. And you'd be 100% right in adding a check here if these messages had any effect on the backend. But, they only affect the frontend, which is inherently (as you correctly point out) untrusted.

Like I said, in a situation where the developer missed some checks, the additional strict checks in the component could help.

No developer using LiveSelect according to the docs is supposed to send those message, so I don't feel the need to raise special error messages in that case. Otherwise, the whole component would be full of those checks.

A malicious actor is a user sending unexpected data in WS messages

• The only way in which you can do that is if you successfully performed an XSS injection attack.
• If you do so, you have complete control of the front-end and can do anything: you don't need to send a message to the LV component in the backend to remove the tags from the selection, you can do it in the frontend
• Therefore, precisely because the front-end is untrusted (with or without malicious intent) I don't feel the need of protecting the front-end from itself.