Add a general abstraction for many of the UI components

[mgarciavaadin]

Describe your motivation

It would be great to have a basic abstraction class or interface gathering most of the features of a component as it was available in Vaadin 8 with the Component Interface.

This new abstraction would have an impact on the modernization of Vaadin 8 to Flow 24 applications where the Component Interface was frequently used. Also for new developments it would provide a common access point to the most frequently used methods related to components and a way to iterate over a same class/interface instance been able to change the style, the label, the size... etc without extra castings.

Describe the solution you'd like

First approach would be to create an interface with the methods from:

• Component public methods
• HasElement
• HasEnabled
• HasLabel
• HasStyle
• HasTooltip
• HasSize

(Edited: HasValue discarded as V8 Component does not implement it. )

Then this abstraction would be applied to all classes that implement HasLabel, which covers the smallest subset of Flow components implementing these 5 interfaces listed above.

The list so far is:

• AbstractNumberField
• BigDecimalField
• Checkbox
• CheckboxGroup
• ComboBox
• ComboBoxBase
• CustomField
• DatePicker
• DateTimePicker
• EmailField
• IntegerField
• MultiSelectComboBox
• NumberField
• PasswordField
• RadioButtonGroup
• Select
• TextArea
• TextField
• TextFieldBase
• TimePicker

(Edited: Tab discarded as it does not implement all the goal interfaces.)

Describe alternatives you've considered

No alternatives considered

Additional context

The intention is that this new feature won't have impact on existing apps as components affected should keep implementing the same interfaces just having a higher level common interface available.

For new implementations the developer would be able to call methods from the collected interfaces without additional castings. i.e.:

        // listComponents is a list with different types of components such as ComboBox, Checkbox, CheckboxGroup, etc...
        for (CommonComponentInterface cm : listComponents){
                  //HasLabel
                  cm.setLabel("");
                  //HasSize
                  cm.setHeight("");
                  //HasEnabled
                  cm.setEnabled(false);
                  //HasStyle
                  cm.addClassName("");
                  //HasTooltip
                  cm.setTooltipText("");
        }

[knoobie]

Naming ideas: Field if you guys also wanna add HasValue to the list of interfaces (and remove HasSize), which would also improve e.g. the usability for users when e.g. iterating over form layouts "fields" where a method could be provided to return "all fields" of the FormLayout which would just simply check if the component is an instace of this new interface.

The same "could" be done for layout-based components like HorizontalLayout, Div, FlexLayout and so on, where e.g. a common interface could provide add/remove and sizing properties and (of course) easier distinguishing between Fields and Layouts inside a any Component.

/cc @rolfsmeds

[mgarciavaadin]

Naming ideas: Field if you guys also wanna add HasValue to the list of interfaces (and remove HasSize), which would also improve e.g. the usability for users when e.g. iterating over form layouts "fields" where a method could be provided to return "all fields" of the FormLayout which would just simply check if the component is an instace of this new interface.

The same "could" be done for layout-based components like HorizontalLayout, Div, FlexLayout and so on, where e.g. a common interface could provide add/remove and sizing properties and (of course) easier distinguishing between Fields and Layouts inside a any Component.

/cc @rolfsmeds

We were thinking on 'InputField' as name so we are close ;) Thanks!!

Yes on the first approach we have included HasValue and discarded Tab as it is not implementing HasSize/HasValue.

I like what your are suggesting about XLayout::getFields() but I'm not getting the idea of your comment about removing HasSize from the collected interfaces, What would be the advantage of that?

[knoobie]

HasSizecontains e.g. setHeightand other methods that are not / rarely used within fields (in my opinion) with the exception of setWidth. I would not clutter the common interface in the first iteration with soo many methods.

[sissbruecker]

@mgarciavaadin Can you please elaborate how this would be beneficial for migrations, especially for automated ones like the V8 upgrade automation? It doesn't seem to make sense to adapt code that iterates over a collection of Components to something that iterates over a collection of InputFields because not every component is going to be an input field. It would be interesting to see where you want to go with this, and why casting to the more granular interfaces is not an option.

Another concern is that adding such a catch-all interface increases the chance to introduce broken APIs in the future, because a component simply doesn't support one of the combined interfaces. We could either miss that during development and ship something that's broken, or we figure it out beforehand, and ship something that doesn't support the new interface, which looks just as broken to users. I'm lacking perspective on the history of Flow development, but it feels like the more granular interfaces were favored over the old catch-all interface just for that reason.

[mgarciavaadin]

@mgarciavaadin Can you please elaborate how this would be beneficial for migrations, especially for automated ones like the V8 upgrade automation? It doesn't seem to make sense to adapt code that iterates over a collection of Components to something that iterates over a collection of InputFields because not every component is going to be an input field. It would be interesting to see where you want to go with this, and why casting to the more granular interfaces is not an option.

So about automatic migrations, so far we have no alternative for the V8 Component interface in Flow. Even if it is not perfect because as you say is not going to match every case we would like to have something in Flow similar to that interface as close as it could be possible. The most frequent use case we have detected in V8 apps is the use of this interface around a container (usually a form) and modifying somehow the fields inside i.e.: setting a style, a caption or visibility. So with this change we would expect to deal with a significant use cases of V8 Component references.

Another concern is that adding such a catch-all interface increases the chance to introduce broken APIs in the future, because a component simply doesn't support one of the combined interfaces. We could either miss that during development and ship something that's broken, or we figure it out beforehand, and ship something that doesn't support the new interface, which looks just as broken to users. I'm lacking perspective on the history of Flow development, but it feels like the more granular interfaces were favored over the old catch-all interface just for that reason.

I hope that should not happen because we are not removing the granular interfaces just creating a wrapper that reuses implementations from interfaces and some Flow Component methods. Anyways I'm also lacking this perspective, but in my humble opinion adding catch-all interfaces when they are possible could help users to work with Vaadin (specially not advanced ones).

/cc @bennewi

[rolfsmeds]

Even with the more granular, existing interfaces left in place, there's still the risk of creating a broken-seeming API: Let's say we add an InputField interface, with APIs that all current input fields support, but later introduce an input field that, for whatever reason, cannot support all those APIs. We then have to choose between broken APIs or having that particular input field not implement the InputField interface, which would probably be perceived as broken or at least quite unexpected and confusing to developers.

We already have parallels to this situation e.g. with the addition of HasStyle to Component class, since e.g. Text, Dialog and ContextMenu don't actually support it (because Text is not an element so can't have styles, and Dialog etc is an overlay so inline styles would need to be automagically forwarded to it from the inline html element).

[bennewi]

We are deliberately targeting a subset of all Components for this, even from the start; the selection is based on commonly used components and we are focusing on the HasLabel interface as the interface that the smallest number of fields support. We can accept that components that are subsequently added may fit specific use cases that put them outside of the scope of this new interface.

The decision to bring HasStyle to Component and not HasSize does not seem consistent to me with the design goal of keeping the base light. What we are aiming for with this change succeeds in:

• keeping the base Component light, while
• giving us a single way of interacting with the most methods possible from the most-used form components

The primary use case is to cover dynamically assembled views where some logic runs that based on a condition would iterate through a Collection of these CommonComponentInterface which has some utility as opposed to a Collection of Components which is significantly limited when compared to Vaadin 8.

[rolfsmeds]

A code example from a to-be-migrated V8 app might be helpful to exemplify the use case.

[web-padawan]

To me it sounds like we need a broader discussion on this topic unless tentative agreement has already been reached. Let me invite the Flow team members @mshabarov @mcollovati

We are deliberately targeting a subset of all Components for this, even from the start;

This is exactly what I'm personally concerned about. Of course I might be wrong as Java expertise is not my main strength. But I would like us to cone up with a general solution.

[mgarciavaadin]

An example of the migration target would be something like this

@Theme("mytheme")
public class MyUI extends UI {

    @Override
    protected void init(VaadinRequest vaadinRequest) {

        FormLayout mainLayout = new FormLayout();

        CheckBox cb = new CheckBox("Checkbox");
        CheckBoxGroup<String> cg = new CheckBoxGroup<>("Multiple Selection");
        cg.setItems("Many", "Muchos", "Monta");
        ComboBox<String> cob = new ComboBox<>();
        cob.setItems("option1", "option2", "option3");
        DateField dp = new DateField();
        DateTimeField dtp = new DateTimeField();
        PasswordField pwf = new PasswordField();
        RadioButtonGroup<String> rbg = new RadioButtonGroup<>();
        TextArea tar = new TextArea();
        TextField txf = new TextField();

        mainLayout.addComponents(cb, cg, cob, dp, dtp, pwf, rbg, tar, txf);

        int i = 0;
        Iterator<Component> iterator = mainLayout.iterator();
        while (iterator.hasNext()) {
            Component field = iterator.next();
            field.addStyleName("my-field-class");
            field.setCaption("Field " + i++);
            field.setWidth("100%");
        }
        setContent(mainLayout);
    }

    @WebServlet(urlPatterns = "/*", name = "MyUIServlet", asyncSupported = true)
    @VaadinServletConfiguration(ui = MyUI.class, productionMode = false)
    public static class MyUIServlet extends VaadinServlet {
    }
}

[rolfsmeds]

Right, so the purpose of the interface would be to avoid having to rewrite that to

while (iterator.hasNext()) {
    Component field = iterator.next();
    if (field instanceof HasStyle hst) hst.addClassName("my-field-class");
    if (field instanceof HasLabel hlbl) hlbl.setLabel("Field " + i++);
    if (field instanceof HasSize hsz) hsz.setWidth("100%");
}

Someone cleverer in Java than me might be able to propose an alternative that would make migration (automated or manual) easier without refactoring all those components to implement a potentially problematic interface, and maybe even solve the problem for other APIs in the V8 Component class as well.

(I'm not categorically against the proposed interface, but would be good to see if there are alternatives first, especially if they might be able to solve a wider range of migration hassles.)

[knoobie]

Personally I like the idea of a centralized interface(s) a lot, that's why I suggested and kinda pested you guys about a flow-components-shared package in the past. That's why I know and hopefully understand where the problem of the component team comes from - they don't want to create a another "swiss army knife" interface that has literally hundreds of methods and can be used for all of kind things (good and bad) - resulting in a total nightmare to maintain.

On the other side, I would be very happy to get such common interface(s) to ease migration and development of complex business apps where I'm often creating my own abstraction on top of flow components to reduce the amount of code I have to create because of repeating tasks for multiple fields.

Long story short.. instead of creating a super interface which has all the methods from all the interfaces.. why not FOCUS on the important methods and create a single interface with methods matching the other interfaces, so that the components could implement this interface and the "full fledged" original interfaces.

In my mind, I'm normally doing this to ALL fields:

• get/set label (HasLabel)
• get/set value (HasValue)
• get/set helper (Checkbox.. I still miss this :/) (HasHelper)
• set width (no interface)
• get/set enable (HasEnabled)
• get/set read only (no interface)
• get/set visible (no interface)
• get element (for people needing to deep dive) (HasElement)
• add class name (no interface)

That would (in my opinion) be the smallest set of methods ANY input field should satisfy for consistency. Even if you guys are going to add another type of field, those "basic" features should be always there.

'* no interface means either: there is no interface for this method or don't implement it on "InputField" because it has way to many unrelated methods and just add this method into the InputField interface directly

[web-padawan]

Decided to have a meeting next week to figure out what to do next and to reach an agreement.

Until then, no PRs can be merged without approval from the Design System team and / or Flow team.

[mcollovati]

Personally, don't like that much having components implement this huge interface, but like the composition of the HasXXX interfaces.

As a random thought, I wonder if the same goal could be achieved by having this new interface acting as a decorator over the existing component and just delegate calls to the HasXXX interfaces

CommonComponentInfertace.of(component).setLabel("").setEnabled(false)

Implementation of a single method may be a NO-OP, if the wrapped component is not implementing the related interface, or it may throw an exception.

The factory method for CommonComponentInterface may also return an optional, empty if the wrapped component does not implement all expected interfaces. So, it should also be possible to have an iterator method that only returns wrapper for suitable components.

However, I don't know if this can help to simplify modernization and daily usage for Flow dev, or if it will make things more complicated.

[rolfsmeds]

Thanks @mcollovati – that's better than the "ComponentIterator" idea I had myself.

So to underline the point here: That solution could work for any component interfaces, not just the subset common to input fields, and you could feed any component to it, not just input fields.

The name could of course be something significantly more compact than that.

[sissbruecker]

I like that approach as well because it looks like it would scale better to adapt to new interfaces or components. The downside is that the DX is not as good, because you don't get any feedback at development time whether a call like setLabel will actually work or not, and it's a bit less convenient to use.

[rolfsmeds]

As for the proposed InputField interface, I'm weirded out by the fact that it defines methods that are part of the Component class, but the Component class of course doesn't implement InputField, so the definition and the base implementation (which may or may not be overridden by individual components) are completely disconnected.

I'm not sure if that's a real problem, but it has a funny smell to me.

HasElement also feels like a problematic inclusion, since it's a low level API that is all too often misused in creative ways. I would not want to promote it like that.

The rest of the list seem much less controversial to me, as they are all fairly obvious input field features, and unlikely to have to be excluded from future components: HasEnabled HasLabel HasStyle HasTooltip HasSize

[bennewi]

Hi this discussion went faster than I was able to keep up with last week. Two comments:

1) this is part of an internal 6 week project that was discussed and agreed upon with 3 Vaadin POs and 2 Vaadin PMs a few weeks ago. The specification and analysis for it can be found in our "products shared" folder with the PRD "V8 Close Gap". Note there are many proposals in this document and this might seem similar to but is completely different from the one proposing an AbstractTextField. So to reply to @web-padawan yes we did go through this 2) Miguel made a copying error from the PRD document to this github issue. The HasElement was not considered to be part of the list of mixin interfaces we're taking into account. So @rolfsmeds yes, your comment is right it shouldn't be there

[mshabarov]

For a long-term solution, the common interface looks better to me, because it gives a better developer experience when you want to use polymorphism benefits to iterate through the components in a container (form layout as a main case here), by just calling existing methods in the components and not thinking of an additional factories or factory methods. Even outside of modernisation needs, this approach would be more convenient in my opinion.

Current minimum list of included interfaces HasEnabled, HasLabel, HasSize, HasStyle, HasTooltip looks good to me, although we can also consider HasHelper interface because it is relevant to input fields nature, where users often needs a suggestions about fields usage, and HasValue as discussed above (despite V8 hasn't it in the interface).

I can't figure out an example of input field component that potentially would not be able to implement some of these basic interfaces. Even if it wouldn't, I suppose it would be a rare case, for which we can throw "not supported exception".

[rolfsmeds]

HasHelper is missing in Checkbox, so we might want to leave it out for now. (The common interface can of course be amended with more stuff later without breaking changes.)

Leaving Component out of the interface, I could also imagine having a static method in InputField that takes a Component stream and returns a Component & InputField stream (filtered to only contain matching components), something like

static <C extends Component & InputField> Stream<C> getInputFieldComponentStream(Stream<Component> components)

usage:

InputField.getInputFieldComponentStream(form.getChildren()).forEach(c-> {
  c.addClassName("my-field-class");
  c.setLabel("Field " + i++);
  c.setWidth("100%");
});

I figure that would help with the most common use case that we're trying to solve here: iterating over a bunch of fields and using common APIs, while keeping the InputField interface conceptually "clean".

[Legioth]

What about replacing InputField.getInputFieldComponentStream(form.getChildren()) with form.getChildren(InputField.class) (defined in Component alongside the current getChildren() method)?

That approach would be less application code to write, more discoverable, and also applicable for other cases.

[rolfsmeds]

Alright, PO decision: Approved to introduce InputField interface collecting the following functional interfaces:

• HasEnabled
• HasLabel
• HasStyle
• HasTooltip
• HasSize

(HasSize is a bit problematic since it's not meaningful to set a height on most field components, but I would include it as it is quite common to setWidth, which is defined in the same interface.)

(Note that we're specifically excluding previously proposed HasElement and methods from Component, and, for now, also HasHelper to see if we might actually add support for it in Checkbox first.)

and to refactor the following component classes to implement it instead of the ones it replaces:

• BigDecimalField
• Checkbox
• CheckboxGroup
• ComboBoxBase
• CustomField
• DatePicker
• DateTimePicker
• EmailField
• IntegerField
• MultiSelectComboBox
• NumberField
• PasswordField
• RadioButtonGroup
• Select
• TextArea
• TextFieldBase
• TimePicker

(Note that TextField and ComboBox are excluded in favor of their *Base classes – couldn't see a need to have it in both, but comment if you have one.)

[knoobie]

https://github.com/vaadin/web-components/issues/3749 here is the issue about Checkbox Helper support ;)

Edit: Can you share the reason why HasValue was excluded?

[mgarciavaadin]

Alright, PO decision: Approved to introduce InputField interface collecting the following functional interfaces:

* [HasEnabled](https://vaadin.com/api/platform/24.0.4/com/vaadin/flow/component/HasEnabled.html)

* [HasLabel](https://vaadin.com/api/platform/24.0.4/com/vaadin/flow/component/HasLabel.html)

* [HasStyle](https://vaadin.com/api/platform/24.0.4/com/vaadin/flow/component/HasStyle.html)

* [HasTooltip](https://vaadin.com/api/platform/24.0.4/com/vaadin/flow/component/shared/HasTooltip.html)

* [HasSize](https://vaadin.com/api/platform/24.0.4/com/vaadin/flow/component/HasSize.html)

(HasSize is a bit problematic since it's not meaningful to set a height on most field components, but I would include it as it is quite common to setWidth, which is defined in the same interface.)

(Note that we're specifically excluding previously proposed HasElement and methods from Component, and, for now, also HasHelper to see if we might actually add support for it in Checkbox first.)

and to refactor the following component classes to implement it instead of the ones it replaces:

* BigDecimalField

* Checkbox

* CheckboxGroup

* ComboBoxBase

* CustomField

* DatePicker

* DateTimePicker

* EmailField

* IntegerField

* MultiSelectComboBox

* NumberField

* PasswordField

* RadioButtonGroup

* Select

* TextArea

* TextFieldBase

* TimePicker

(Note that TextField and ComboBox are excluded in favor of their *Base classes – couldn't see a need to have it in both, but comment if you have one.)

It sounds great! Thanks to everyone for participating on the discussion and POs for the effort to get to an agreement!

Just a couple of comments:

• Are we all ok to add the method 'InputField.getInputFieldComponentStream' or 'Component.getChildren(Class)' as @Legioth suggested? Second option seems to be more general I'd vote for that one.
• Any idea/suggestion/change for the name of the interface? as @knoobie pointed out the word 'Input' without implementing HasValue kind of weird. Maybe we should just call the interface 'Field'.

Thanks all again!

[bennewi]

OK some progress here on defining a larger interface to highlight the core substitutability of "what it means to be a Vaadin field component", but we're still falling short of the original intent of the spec to include Component and we've got a half and half situation which will still lead us to needing casting.

If the concern of the bad smell of Component not implementing InputField, I'd suggest we address the issue as follows:

• Create a new ComponentCore interface that corresponds to the methods of Component
• Redefine Component to implement ComponentCore
• Redefine InputField to extend ComponentCore

[Legioth]

Methods defined in Component can be used thanks to the generic type of the suggested getChildren or getInputFieldComponentStream method.

[bennewi]

If I understand correctly, this proposal with a new getChildren will work if the use case is only to iterate over children and the children all have the same parent. But the case we're looking to cover with the single interface includes things like Java ArrayLists of Components. These ArrayLists could be a subset of the children of a container or children of multiple containers. I'm still looking for a single interface.

We also want to have the interface as a formal parameter to a method.

[Legioth]

If I understand correctly, this proposal with a new getChildren will work if the use case is only to iterate over children and the children all have the same parent.

Good point. That's a benefit of getInputFieldComponentStream since then you can more easily combine items from multiple sources.

[rolfsmeds]

Yes, I do think that either the getChildren overload or the stream method would be generally beneficial (not just for migrations), so I'd be happy to see that implemented in a separate PR.

As for the name and HasValue, I don't see any obvious reason to exclude it – just forgot about it when assessing this. Unless @yuriy-fix or @Legioth see any issues with it, I'd propose including it.

[mgarciavaadin]

So if there are no cons, HasValue would be included. Actually HasValueAndElement that extends HasValue is proposed in the solution PR as it is a higher common interface for this list of components.

[rolfsmeds]

Yes but that includes Element which we Do Not Want. Let's add HasValue itself.

[knoobie]

HasTooltip already extends HasElement (Edit: all other interfaces extend it) :) So HasValueAndElementor HasValueshouldn't change it the outcome in the end, even tho it feels more consistent to only have HasValue instead of the currrent HasValueAndElement.

[rolfsmeds]

Huh. We might want to remove HasElement from HasTooltip. Doesn't seem to me that it should be there.

[Legioth]

Removing HasElement from HasTooltip is not trivial since the default methods in HasTooltip use getElement().

[mgarciavaadin]

HasElement is included in HasStyle and HasLabel also. Anyways in PR HasValueAndElement has been replaced by HasValue on the final proposal after discussion with @sissbruecker
/CC @rolfsmeds