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

[grid-pro] Custom component edit fields contains value of previous edited row for a short moment, when switching edit #2550

Closed
stefanuebe opened this issue Sep 16, 2021 · 13 comments · Fixed by vaadin/flow-components#6820 or #8156
Assignees
Labels
BFP Bug fix prioritised by a customer bug Something isn't working Impact: High needs design Design is needed Severity: Minor vaadin-grid-pro

Comments

@stefanuebe
Copy link

Description

(Preamble: Not sure, if this is a Flow or web component issue. :) )

Edit columns with custom fields show for a very short amount the value of the previously edited row, when switching between two rows. While it is not really a technical issue, it is at least not very appealing visually. This seems to be due to the activation of the edit mode on the client and the async request of the value to show on the server (which takes in the worst case some 100 ms).

Expected outcome

The newly edited field should be empty until the new value is fetched from the server. For this, the grid pro could fetch the default empty value of the custom field and try to apply that first before activating it for the next row to edit.

Minimal reproducible example

@Route(value = "grid-pro-custom-field-issue", layout = MainLayout.class)
public class GridProCustomFieldIssuesView extends Div {
    public GridProCustomFieldIssuesView() {
        List<SamplePerson> persons = new ArrayList<>();

        for (int i = 0; i < 10; i++) {
            persons.add(new SamplePerson("Person " + i));
        }

        GridPro<SamplePerson> grid = new GridPro<>();
        grid.setEditOnClick(true);
        grid.setItems(persons);

        TextField component = new TextField();
        grid.addEditColumn(SamplePerson::getName)
                .custom(component, SamplePerson::setName)
                .setHeader("Name");

        add(grid);
    }
    public static class SamplePerson {
        private String name;

        public SamplePerson(String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }
    }
}

Steps to reproduce

  1. Start editing a row
  2. Use tab to move to the next row
  3. For a brief moment, the field will show the old value, before getting the new one fetched from the server.

Environment

Vaadin 21, Chrome 93

@vursen
Copy link
Contributor

vursen commented Nov 1, 2021

Confirmed that the issue can be reproduced in V14.7.3 as well as the latest V22.0.0.alpha9 so that it isn't a regression.

Tried out to reproduce the issue with different editor components. The issue only occured for custom editor components but it didn't for the built-in editor components:

  • .custom(new TextField(), SamplePerson::setName) – reproducible
  • .custom(new TextArea(), SamplePerson::setName) – reproducible
  • .text(SamplePerson::setName) – not reproducible

Here is the video with the TextField used as a custom editor component (V22):

Screen.Recording.2021-11-01.at.16.14.58.mov

@vursen vursen added bug Something isn't working vaadin-grid-pro labels Nov 1, 2021
@yuriy-fix yuriy-fix added the needs research More information needed to estimate label Feb 24, 2022
@yuriy-fix yuriy-fix changed the title [grid-pro] Custom component edit fields contains value of previous edited row for a short moment, when switching edit [grid-pro] Custom component edit fields contains value of previous edited row for a short moment, when switching edit [2 days] Feb 24, 2022
@vursen vursen assigned vursen and unassigned vursen Mar 3, 2022
@sissbruecker
Copy link
Contributor

This is a Flow component issue. The observed delay is the server roundtrip between showing the editor when cell editing starts, and updating the editor with the edited value from the server.

This also implies that any ideal solution to this problem will need to avoid a server roundtrip, and run some client-side logic as soon as cell editing starts. Resetting the value of the editor to "empty" is not a trivial problem, because we are dealing with custom elements. Best-case the element has a value property that can be set to null. However there are also components that use other value properties, for example combo box uses selectedItem. Worst case we have a composite component that wraps the actual input into a <div>, or contains multiple inputs. Since the component logic is defined in the Flow component, only the Flow component actually knows how to properly apply a value to the custom elements that it contains.

Given that, I can not see any perfect solution to this problem. I can only list some options that come with different trade-offs:

Improve behavior for simple editors

For cases where the editor itself is an input component, such as <input>, <vaadin-combo-box>, or <vaadin-date-picker>, we can add some code that checks if the component has a value-like property, such as value, selectedItem, or checked, and then reset that to a default empty value. The downside is that this solution will not work for composite components, and there might be confusion about the diverging behaviour of simple and composite editors. There is also a concern that resetting the value could lead to a side-effect that modifies the server-side state, which would be critical to avoid.

Hide editors until the value is loaded

Make the custom editor invisible when starting editing, and only show it after the value has loaded. This basically trades the effect of briefly showing the previous value, to the effect of flashing the whole editor. Another concern here is focus management, the web component keeps track of the cell editing state using focus events. So even if the editor is not visible yet, it must already be focused, and must not loose focus until it becomes visible. There might also be accessibility concerns, as the editor still contains the old value, which screen readers might anounce (same problem with the current behavior).

That would kind of look this this:

Bildschirmaufnahme.2022-03-03.um.13.04.12.mp4

Use web components for custom editors

For complex / composite editors the only way to guarantee a contract for setting a value on the client-side is to use a web component that must implement a value property. There would be several ways to go about this implementation-wise (reuse default Grid Pro web component mechanics that gets and sets the value on the client-side, create Flow wrapper and then reset value when starting editing as proposed in the first option). The downside is that this requires to dive into client-side development. We might also want to add new APIs to tell the Flow component to "just use this tag as custom editor".

@sissbruecker sissbruecker added the needs discussion No decision yet, discussion needed label Apr 13, 2022
@TatuLund
Copy link
Contributor

This could be fixed in the GridPro by not removing the Field immediately but upon grid being refreshed.

@probert94
Copy link

We are currently investigating, if the GridPro suits our needs for inline editing of items.
We are currently using the inline editing of the normal Grid but activating the editor needs a server roundtrip and therefor decreases the productivity.
Unfortunately most of our columns need custom components, we can't even use the build in TextField as we need custom client side logic for it.
If we were able to use custom web components as editors in GridPro, that would be a huge benefit and would drastically improve the productivity of our users as they don't need to wait for a server roundtrip before starting to edit the items.

@rolfsmeds
Copy link
Contributor

There is also a closely related issue (caused by the same underlying cause), that the regular (non-edit) value in the cell briefly shows the old (pre-edited) value when the editor is closed, before it is updated with the new value.

@rolfsmeds rolfsmeds added the needs design Design is needed label Nov 11, 2024
@DiegoCardoso DiegoCardoso added the BFP Bug fix prioritised by a customer label Nov 12, 2024
@rolfsmeds rolfsmeds changed the title [grid-pro] Custom component edit fields contains value of previous edited row for a short moment, when switching edit [2 days] [grid-pro] Custom component edit fields contains value of previous edited row for a short moment, when switching edit Nov 12, 2024
@rolfsmeds rolfsmeds removed needs research More information needed to estimate needs discussion No decision yet, discussion needed labels Nov 12, 2024
@rolfsmeds
Copy link
Contributor

So, the plan now is to accept that we cannot do anything about the delay itself, and instead

  • Hide the editor until its value has been updated
  • Hide the cell's readonly value rendering until its value has been updated
  • Show some kind of loading indicator in the cell in the meantime

The design of the loading indicator is still up for discussion. Suggestions welcome.

@anezthes
Copy link
Contributor

Suggestions welcome

Indeterminate circular progress indicator, or something like https://tailwindcss.com/docs/animation#pulse.

@TatuLund
Copy link
Contributor

The design of the loading indicator is still up for discussion. Suggestions welcome.

Lumo does have it already

'@vaadin/vaadin-lumo-styles/mixins/loader.js';

It is used e.g. in TabSheet

@probert94
Copy link

One idea is to fill the editor with the known client-side value and replace it with the server value as soon as it is known.
If the user changes the value in the mean time (i.e. the filed is foucsed and has an "dirty" value), the server value could be ignored.

@tomivirkki tomivirkki self-assigned this Nov 12, 2024
@tomivirkki
Copy link
Member

This was auto-closed by Github automation when merging #8156

Reopening since the issue will actually only be resolved once vaadin/flow-components#6820 is also merged

@enver-haase
Copy link
Contributor

I'd like to re-open. See the video. Vaadin 24.5.

Screen.Recording.2024-12-03.at.13.29.51.mov

@tomivirkki
Copy link
Member

tomivirkki commented Dec 3, 2024

I'd like to re-open. See the video. Vaadin 24.5.

I don't see this issue reproducing in the video (the editor field showing up with a wrong value). This looks more like

  • 6816 (fix not released yet) or
  • 8234 (currently prioritized)

@enver-haase
Copy link
Contributor

Agree that my case looks like vaadin/flow-components#6816 . Please leave closed. I will monitor whether that issue will fix my case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFP Bug fix prioritised by a customer bug Something isn't working Impact: High needs design Design is needed Severity: Minor vaadin-grid-pro
Projects
None yet