vaadin / vaadin-button-flow

Vaadin Flow Java API for vaadin/vaadin-button Web Component
https://vaadin.com/components/vaadin-button
Other
0 stars 7 forks source link

Calling setText for Button appends the text to an existing caption #43

Closed johannesh2 closed 6 years ago

johannesh2 commented 6 years ago

Having a template with <vaadin-button>Click</vaadin-button> And calling later in Java myButton.setText("Me!")

Expected: Button has text "Me!" Actual: Button has text "Click Me!"

pleku commented 6 years ago

We need to find a way to clear that text, as the method is suppose to set the text, not append it.

Legioth commented 6 years ago

The basic issue is that setText is implemented to first remove all children, and then add a single text node. The problem is that removing all children is a no-op in this case when the server-side API isn't aware of the content that is there only through the template. There are thus different obvious ways of approaching this: 1) Make removeAllChildren() actually remove all children, not only those that the server knows about 2) Make the element instance aware of content that is supposedly there because of the template

denis-anisimov commented 6 years ago

The fix of this simple issue which is definitely confusing may introduce terrible consequences.

At the moment it's just a confusion. But after fixing this it may become a catastrophe.

Let's imagine I have :

<template>
<div id="foo>
  Text
   <div id="bar">Bar<div>
</div>
</template>

And I have both divs injected via @Id :

@Id("foo")
private Div child1;
@Id("bar")
private Div child2;

Now we should remove everything inside the first div along with nested div. As a result the child2 refers to non-existing element and there will be huge confusion why it doesn't work.

Also the issue with setText is just one aspect of non-working Element API. The element injected via @Id also doesn't contain children though it may contain children on the client side. And it's the same situation. The theoretical question is : why children methods doesn't work as expected ? Again, if we allow to remove children from the injected element then what happens with those children which has been also injected ?

I would say the main confusion here is @Id concept as a whole. It has not been thought through enough. I would say that Element API (along with Component ) should not be used for injected elements at all. That't the first impression. But in this case it becomes almost useless.

Anyway. What I'm definitely sure: let's don't fix it SOMEHOW. We may introduce much more serious issues than just a confusion as it's for now.

We need to go through all usecases and find solution for each of them. And all of them should be taken into account carefully with a proper design.

This issue requires a serious discussion instead of quick fix proposition.

Legioth commented 6 years ago

child2 would then refer to an element that isn't attached on the client, just as it would if the corresponding DOM structure would be built using only the Element API without any templates. This is what the application developer expects, regardless of whether they're using @Id or not.

There might be some small differences such as the fact that child2 is still attached (as a virtual child) on the server, but that isn't any worse than the situation with ComponentRenderer in Grid where we also can have elements that are attached according to the server even though they are not attached to the actual DOM in the browser.

mvysny commented 6 years ago

@denis-anisimov I agree. The source of the problem as I see it is that the server-side Button (or any Vaadin10 Component for that matter) will not receive any DOM content when created from a Polymer Template. This will cause any API that depend on the DOM content to fail and/or return incorrect results:

I believe this to be a major inconsistency. Not only this kills browserless testing for Polymer Templates, it also makes the server abstraction leak in various ways. The user will have to learn that Buttons and components nested inside of the Polymer Template are inferior than server-side constructed Buttons and behave differently.

pleku commented 6 years ago

This issue was also reported in https://github.com/vaadin/flow/issues/3699.

pleku commented 6 years ago

Based on the amount of feedback on this and on what happens when components added to a Id-mapped layout go to unexpected location, we need to resolve this before RC and release a Beta version with the fix.

Legioth commented 6 years ago

Will be fixed through https://github.com/vaadin/flow/issues/3713

denis-anisimov commented 6 years ago

As I understand this will be fixed automatically once removeAllChildren(); is implemented in the new way.

So may this is only about IT test.