Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DatePicker triggers value-changed when attached #2691

Closed
probert94 opened this issue Feb 17, 2022 · 6 comments
Closed

DatePicker triggers value-changed when attached #2691

probert94 opened this issue Feb 17, 2022 · 6 comments
Assignees
Labels

Comments

@probert94
Copy link

Description

In an application I am working on, I use UI#push to force an update of the client site state (in my case I want to show a loading indicator). Now I noticed, that the value of displayed DatePicker is wrongly set to null.
I found that this is caused by a ValueChangeEvent coming from the client.
After some debugging, I found that a value-changed event is triggered during the ready of the webcomponent, because the value changed from undefined to "".
If the value on the server is still the same at this time, no ValueChangeEvent will be triggered but if the value on the server changed in the mean time, a ValueChangeEvent with fromClient=true is triggered and the server side value is changed to null.
This can even result in a difference between the client side value and the server side value.

The value-changed event on the client also happens for TextField (and probably other fields as well) but the server side ValueChangeEvent only seems to happen with DatePicker.

Expected outcome

There should be no ValueChangeEvent with fromClient=true unless the user manually changed the value on the client side.

Actual outcome

There is an initial value-changed event on the client, which might create a ValueChangeEvent on the server, if the server value changed in the mean time.

Live Demo (optional)

Notification that shows, when the server receives a ValueCHangeEvent from client:
image
It triggers immediately after opening the page, basically as soon as the Datepicker is attached.

Pressing the Show values button shows the server side values:
image
The date on the server is now null, while the client shows the expected value (the current date).

Minimal reproducible example

  private DatePicker dp;
  private TextField tf;

  public EmptyView() {
      dp = new DatePicker();
      dp.addValueChangeListener(e -> {
          if (e.isFromClient())
              Notification.show("Date change from client: " + e.getValue());
      });
      tf = new TextField();
      tf.addValueChangeListener(e -> {
          if (e.isFromClient())
              Notification.show("Text change from client: " + e.getValue());
      });
      Button btn = new Button("Show values", e -> {
         Notification.show("Date: " + dp.getValue());
         Notification.show("Text: " + tf.getValue());
      });
      add(dp, tf, btn);
  }

  @Override
  protected void onAttach(AttachEvent attachEvent) {
      super.onAttach(attachEvent);
      attachEvent.getUI().push();
      dp.setValue(LocalDate.now());
      tf.setValue("Hello world");
  }

Steps to reproduce

  1. Download a Vaadin 22 starter with only an EmptyView
  2. Enable Push in the Application
  3. Copy the above code into the EmptyView
  4. Run the project and open the page, it will show a notification that the date has changed
  5. Press the Show values button and it will show a notification with the current server side values.

Environment

  • Vaadin 22.0.5
  • Windows 10 x64
@yuriy-fix
Copy link
Contributor

Could be related: #1360

@vursen vursen self-assigned this Mar 7, 2022
@vursen vursen assigned vursen and unassigned vursen Mar 10, 2022
@vursen
Copy link
Contributor

vursen commented Mar 15, 2022

Confirmed that the issue is reproducible for both DatePicker and TimePicker, and I guess for DateTimePicker as well, but I didn't check that.

The described behavior comes out of the fact that DatePicker's default value is different on the client-side and the server-side. It is an empty string on the client-side and null on the server-side whereas TextField's default value is an empty string on both and that is why TextField is not affected.

What happens in the example in detail:

  1. Server: Adds DatePicker with no value
  2. Server: Pushes
  3. Client: Initializes the DatePicker web component with the value null
  4. Client: Polymer fires value-changed with null => "" because this is the default value from the web component's point of view.
  5. Client: In response to that, Flow Client schedules a round-trip request to sync the value on the server-side.
  6. Server: Sets DatePicker value to 01.01.2022 and releases the initial HTTP request
  7. Client: Sets the DatePicker web component value to 01.01.2022
  8. Server: Starts processing the scheduled round-trip request with the updated value.
  9. Server: Updates DatePicker value to null because it cannot parse the empty string into a date.

What causes the issue in DatePicker / TimePicker?

As you can see in the following fragment of code, DatePicker skips setting the presentation value when initialValue is null:

if ((getElement().getProperty("value") == null
|| !isInitialValueOptional) && initialValue != null) {
setPresentationValue(initialValue);
}

...and because the DatePicker constructor passes null as initialValue when no custom value is provided, the condition above cannot be met which results in the presentation value branch not getting executed.

Why does the issue not concern TextField?

Although TextField has a similar logic in the constructor:

if ((getElement().getProperty("value") == null
|| !isInitialValueOptional) && initialValue != null) {
setPresentationValue(initialValue);
}

...the difference is that its constructor always passes "" as initialValue when no custom value is provided. This matches the condition above and the presentation value branch gets executed.

@sissbruecker
Copy link
Contributor

sissbruecker commented Mar 15, 2022

Did a quick test and can verify that initializing the Flow component's value property with an empty string fixes the issue.

I remember a similar change for the Select Flow component, where we needed to make sure that the Flow components value property is aligned with the default value of the web component: https://github.com/vaadin/flow-components/pull/973/files#diff-1c027c9076652ce3a2c641ac7407c10cc3a87846a37ee152c27aa9fe59aad609R126-R129. So this is probably a reasonably approach.

Apart from that I'm a bit confused why you would call push during onAttach, which seems to be causing the value desync. That seems like asking for trouble. But maybe that's just a simplified reproduction of a separate thread pushing while the UI thread attaches.

As a side note, I think it's kind of problematic that we rely on the value-changed Polymer event, which always triggers when the web component initalizes. If we could use a custom event that would only be dispatched when the user actually changed something, that would make the value synchronization easier.

@probert94
Copy link
Author

I'm a bit confused why you would call push during onAttach , which seems to be causing the value desync. That seems like asking for trouble.

It's indeed only simple way to reproduce the issue.

@sissbruecker
Copy link
Contributor

It's indeed only simple way to reproduce the issue.

Cool, thanks for confirming.

@vursen
Copy link
Contributor

vursen commented Mar 18, 2022

The issue has been fixed. Stay tuned for V22.0.10 which will be out soon.

@vursen vursen closed this as completed Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants