Flow: Make setEnabled Great Again

[pleku]

EDIT: see https://github.com/vaadin/flow/issues/3538#issuecomment-378172077 for design details.

Any component that has the disabled property, should implement the HasEnabled mixin-interface. For those components, we should prevent any property change updates and events coming from client side.
It should be possible to specify some whitelisted properties and events that are allowed, eg. if the component allows some updates to be sent to server like scrolling position changes.

For now, this is not propagated in any way, the parents would have to specifically apply this to its children when needed. We don't currently have any layouts that have disabled property.

This should be handled only on the server side. Please make it clear in the javadocs that the disabling of the component only applies after the next response (and make it work like so).

[Legioth]

Another question is how to deal with propagation of the enabled/disabled state. In FW8, all descendants of disabled components are automatically also disabled. If HasEnabled maps to the HTML disabled attribute, then we would either have to manually ensure that the attribute is propagated to all children, or alternatively implement a more limited feature that would also cause surprises for anyone familiar with older framework versions.

Recursive applying of the attribute would also have to automatically cover elements that are attached or detached after setEnabled(false) has already been run.

[pleku]

Recursive applying of the attribute would also have to automatically cover elements that are attached or detached after setEnabled(false) has already been run.

Decided to leave this up to the component container to implement this.

[Legioth]

Decided to leave this up to the component container to implement this.

So this feature will be working randomly from the application developer's perspective?

[pleku]

Decided to leave this up to the component container to implement this.

So this feature will be working randomly from the application developer's perspective?

No randomness. We don't currently have any components containers that support this. We cannot force anyone to implement it with one way regardless. We have https://github.com/vaadin/flow/issues/3546 where we can set an example where it works by applying it for children.

One could have a component container where eg. on part is disabled (a button for remove) and another part is not (a button for open card/details/expand) and there automatic apply-to-all children disabled would not make sense.

[Legioth]

From the user's point of view, it's random when refactoring the layout suddenly also causes setEnabled to work in a different way.

What we can do is to universally apply this on the element level, regardless of any components, and then provide hooks for the component implementations that want disabledness to only be applied selectively.

[pleku]

Maybe. I'm still not convinced though that it would be the best solution. Lets see what the team thinks and what feedback there would be when we get to this (I'm not expecting any though).

For the implementation side it feels to me bad to apply disabled to the element in one level and then having to apply it for all the N child elements in the DOM, where most of those probably it won't mean anything. I would rather focus on this on a component level for those components where it actually makes sense.

[pleku]

There needs to be a tutorial for this in the component features and link to that tutorial in the v8-v10 migration documentation. In the latter also the "in progress" note needs to be removed, just search for this issue number.

[pleku]

The issue for applying this for our component containers: https://github.com/vaadin/flow/issues/2277

[Legioth]

I didn't mean that we would set the disabled attribute on all elements, but rather automatically propagate the setting throughout the server-side element tree so that it would automatically work despite lacking support in intermediate wrapping components.

[mstahv]

Without this feature our promise of improved security gets a big hit. I really hope this is solved before the final is out.

[samie]

I think @mstahv has a good point here. Philosophy from previous Vaadin versions has been that application code runs securely on server, and one part of that has been that disabled and hidden components ignore any client-side events to avoid potential client-side component hacking. Now, if the component events are not filtered by the framework (or at least components), it exposes the applications for whole new attack surface. These kind of problems are then very hard to spot and possibly also impossible to fix on application level.

[Legioth]

One alternative based a separate discussion would be to add three methods to Component:

• void setEnabled(boolean)
  
  
  
  • Only affects the component itself by setting a disabled attribute on the element and blocking any updates from the client. Blocking of updates is only based on the component's own setting, i.e. doesn't consider the setting of parent components.
  
  
• void setEnabledRecursively(boolean)
  
  
  
  • Runs setEnabled(value) for the component itself and all currently attached descendants (based on the server-side DOM structure).
  
  
• boolean isEnabled()
  
  
  
  • Checks the setting for the component itself without considering parents.
  
  

Some observations on the implications of this alternative:

• Components attached after setEnabledRecursively would not be affected
• A component that has been marked as disabled through setEnabledRecursively wouldn't remain disabled even if being moved to some other part of the hierarchy.
• The API would be available on all component instances, which makes it possible to e.g. disable all controls in a form by doing setEnabled(false) on the enclosing layout component.
  
  
  
  • If desirable, there could be some way for component implementations to configure whether the disabled attribute should be set in the client. This could be e.g. a marker interface, an annotation or a method to override with an alternative implementation.
  
  
• There is at least one situation when the server-side hierarchy doesn't fully match expectations, and thus also affecting how setEnabledRecursively behaves. This is that all @Id injected components have the PolymerTemplate as their component parent even though the elements would be nested inside each other based on the template.
• @EventHandler would be disabled based on the status of the PolymerTemplate instance and not on the status of the descendant component that has an on-event-name="handlerName" declaration.

[samie]

Second point should probably then be: _Runs setEnabled(value) for the component itself and setEnabledRecursively(value) to all currently attached descendants_

Most important thing here is that whatever way it is done, it is done on the server component tree and not only on the client-side to avoid any client-side modification issues (I.e. if it looks disabled, it also is disabled).

[Legioth]

Second point should probably then be: _Runs setEnabled(value) for the component itself and setEnabledRecursively(value) to all currently attached descendants_

Actually not, but we're splitting hairs here. _Descendants_ include the entire sub tree and not only the component's own immediate children. It's thus either setEnabled for all _descendants_ or setEnabledRecursively for all _children_.

Out of those, the former definition allows more freedom in how the feature is actually implemented (e.g. without recursion to avoid filling up the call stack with deep component hierarchies), but it still doesn't rule out an implementation that calls setEnabledRecursively for all children.

[samie]

Ok, now I see what your looking here, but the problem is that this implementation that you describe not "recursive", but "flattening" and does not really match the expectations from the method name.

The use case I'm thinking here is component encapsulation. That as a _component developer_ I still want to be in full control which parts of my composite component (i.e. sub-components) are really enabled/disabled and when. There are situations where a complex component is partly functional even in disabled state. Example of this might be disabled custom list component that still allows browsing, but not clicking or editing of items. We cannot know (should not need know) that from parent component.

[Artur-]

Do keep in mind that this is a very much used pattern:

Layout

• Child layout
  
  
  
  • Child component 1
  
  
  • Child component 2 (disabled)
  
  

When an event is triggered you disable the whole layout, e.g. layout. setEnabledRecursively(false)

Layout (disabled)

• Child layout (disabled)
  
  
  
  • Child component 1 (disabled)
  
  
  • Child component 2 (disabled)
  
  

When another event is triggered you enable the whole layout again, e.g. layout. setEnabledRecursively(true)

Now at this point, you really really want "Child component 2" to still be disabled and be back in the original state. The enabled/disabled state of "child component 2" is tied to something completely different than the layout. Having to manually keep track of this would be way too much work.

[pleku]

Disabled state blocks the following client->server changes (this part is same with setVisible) :

• property change (model sync)
• dom events
• event handler
• client delegate

Practical implications

• Any state node should react automatically to disabled state change and apply the restrictions
• Any component should inherit the disabled state from the parent.
• Any component, that can be set as disabled, should implement HasEnabled
• HasEnabled should apply the attribute also by default
• Having a ComponentRenderer with buttons (eg. copy/delete/edit), the buttons should be disabled when the grid/column is disabled, meaning that the attribute is applied and no changes have any effect
  -> all preexisting renderers (not templaterenderer) should be applying the rules accordingly (and tested to do so)
• HasComponents extends HasEnabled, thus making any layout have the API for disabling it
• HasValue extends HasEnabled

Corner cases

• TemplateRenderer the attribute is not applied without the developer applying it, but disabled state is the state node level
• grid and iron-list don't have any disabled API, which would prevent the web component from scrolling, thus the communication related to scrolling should be working even with a disabled component
  
  
  
  • nothing else should work at the moment
  
  

Implementation details

• Component has protected API that is triggered when the disabled state is changed
  
  
  
  • triggered whenever that the state node setting has changed or when the parent state node setting has changed, or whenever the component is attached/detached
  
  
  • default implementation applies the disabled attribute if the component implements HasEnabled
  
  
• HasEnabled interface has setEnabled and isEnabled
  
  
  
  • it is applied to the interfaces HasValue and HasComponents and Focusable
  
  
  • all component implementations are separate issues
  
  
• for all entry points that are affected by disabled, it should be possible to say it is OK to handle this communication even if the element is disabled. For declarative API there should be another boolean property for this. For methods there is another more specific overload. The cases are:
  
  
  
  • property change (model sync), both declarative and element API
  
  
  • dom events, both declarative and element API
  
  
  • event handler, declarative API
  
  
  • client delegate, declarative API
  
  

Acceptance Criteria

• There is a tutorial for "How do I make my component apply disabled state"
• There is a tutorial for "How do I allow some user interaction for a disabled component"
• There is a tutorial for "How do I disable a form / layout"
• Implementation is done

[denis-anisimov]

The use case I'm thinking here is component encapsulation. That as a component developer I still want to be in full control which parts of my composite component (i.e. sub-components) are really enabled/disabled and when. There are situations where a complex component is partly functional even in disabled state. Example of this might be disabled custom list component that still allows browsing, but not clicking or editing of items. We cannot know (should not need know) that from parent component.

This looks as a read-only feature description for me.
I'm not sure that disabled should allow anything at all for any composite component.
I'm afraid that we confuse two different features: read-only and enabled.
read-only really should allow some user interactions for complicated components.
disabled should not allow do anything at all. That's the reason I would use disabled against read-only.
Disabled component just shows its presence in UI and explicitly emphasize that there is some additional functionality but it's not available at the moment at all because of another UI aspects (or security). And I may not do _anything_ with disabled component at all until something happens ( another UI control allows this) or access is granted.

So I would say we pay too much attention to this feature allowing some interaction when nothing should be allowed at all.

The problem is that I need to write a tutorial for the feature (which describes a usecase when disabled component should allow some user interaction) and I can't come up with any meaningful usecase at all.
The only usecase I may imagine is scrolling which might be enabled for disabled component .
In reality I think it should not be enabled as well. And definitely any navigation/clicks should not be available.

[Legioth]

That as a component developer I still want to be in full control which parts of my composite component (i.e. sub-components) are really enabled/disabled and when.

There hasn't been any such feature in Vaadin 8, and I don't remember seeing any feedback about that with one exception: We have hardcoded Grid lazy loading to work even when the Grid is disabled, since we didn't want to also disable scrolling. This will be achievable without hardcoding in Flow as well once https://github.com/vaadin/flow/issues/3808 is implemented.

[samie]

I think this has been possible simply by overriding setEnabled? And looks basically the same scenario @Artur- was presenting already, but from other way around, and not for layouts but composite components.

[Legioth]

Overriding setEnabled() might have been possible if the status of the component itself shouldn't be changed, but only propagate it to child components. If the parent's own state changes, then it would at the very least be necessary to also override AbstractComponent.isConnectorEnabled() and on the client AbstractConnector.isEnabled().