GridPro: CustomField<String> as editor does not work if fields are encapsulated by a Dialog

drewharvey commented 3 years ago

Description

Using a CustomField<String> as a custom editor in GridPro works as expected. However, if you wrap the fields in a Dialog the editor does not get/set the values correctly.

Live Demo (optional)

gridpro-editor-dialog-issue

Minimal reproducible example

Working (not Dialog):

public class TestCustomField extends CustomField<String> {

    private NumberField numberField;
    private Button showDialogBtn;

    public TestCustomField() {
        super();

        numberField = new NumberField();
        numberField.addValueChangeListener(e -> setModelValue(generateModelValue(), e.isFromClient()));
        add(numberField);
    }

    @Override
    protected String generateModelValue() {
        if (numberField.getValue() == null)
            return null;

        return numberField.getValue().toString();
    }

    @Override
    protected void setPresentationValue(String newPresentationValue) {
        Double value = null;
        if (newPresentationValue != null) {
            try {
                value = Double.parseDouble(newPresentationValue);
            } catch (NumberFormatException e) {
                // do nothing
            }
        }
        numberField.setValue(value);
    }
}

Not working (fields wrapped in Dialog):

public class TestCustomField extends CustomField<String> {

    private NumberField numberField;
    private Button showDialogBtn;

    public TestCustomField() {
        super();

        numberField = new NumberField();
        numberField.addValueChangeListener(e -> setModelValue(generateModelValue(), e.isFromClient()));

        Dialog dialog = new Dialog();
        Button confirmButton = new Button("OK", e -> {
            setModelValue(generateModelValue(), true);
            dialog.close();
        });
        dialog.add(numberField, confirmButton);

        showDialogBtn = new Button("Edit", e -> dialog.open());
        add(showDialogBtn);
    }

    @Override
    protected String generateModelValue() {
        if (numberField.getValue() == null)
            return null;

        return numberField.getValue().toString();
    }

    @Override
    protected void setPresentationValue(String newPresentationValue) {
        Double value = null;
        if (newPresentationValue != null) {
            try {
                value = Double.parseDouble(newPresentationValue);
            } catch (NumberFormatException e) {
                // do nothing
            }
        }
        numberField.setValue(value);
        showDialogBtn.setText(value != null ? value.toString() : "Edit");
    }
}

Environment

Vaadin: 22.0.0.alpha5

OSX 11.6

Browsers Affected

Only tested in Chrome

sissbruecker commented 3 years ago

I think what you are trying to do here is a bit convoluted and IMO not supported (would need to check with PO). GridPro is about inline editing, which implies that custom fields are placed within the relevant grid cell and have focus. What you want to do here is not inline editing. As such you should move the editing logic out of GridPro. This can be achieved by:

creating a custom column renderer that renders the edit button
clicking on the button opens the dialog with the text field, populates it with the data from the edited item
confirming the dialog updates the item, and refreshes the data provider

drewharvey commented 3 years ago

@sissbruecker I see your point. Let me try this approach. Keyboard navigation is the primary reason I am using GridPro, so I need to see if rendering a button breaks keyboard flow.

sissbruecker commented 3 years ago

Put together a small demo that demonstrates the idea outlined above: https://gist.github.com/sissbruecker/b7709c79ca5e6cdf51594285b641cd6b

Keyboard navigation works as long as the focus stays within the grid. The buttons do not interfere with the tab order. To trigger an edit button with the keyboard, focus the cell with the button, hit Enter, which sets focus on the button. Then hit Enter again to trigger the button.

The only issue is that as soon as the dialog opens, the Grid loses focus, and AFAIK it's currently not possible to programmatically:

restore focus to a specific row or cell
or even focus the Grid, which seems to be not implemented: https://github.com/vaadin/flow-components/issues/2180
(assuming one of the above would work) force Grid to show the focus outline, which is only shown if the focus was changed due to a keyboard event

So after the dialog closes, you would not be able to guide the user back to the place where they initiated the editing.

drewharvey commented 3 years ago

@sissbruecker this is a great starting point that I will use as a workaround for now. Paralell with the double "Enter" press required, you will also need to double Click in order to activate the Edit button (if you are not using keyboard nav). I still think using the .custom api with something like this would be nice. Even if you need to manually fire events or whatever to notify the grid of changes.

sissbruecker commented 3 years ago

Paralell with the double "Enter" press required, you will also need to double Click in order to activate the Edit button (if you are not using keyboard nav)

When using a custom renderer with addColumn, the button is immediately visible and clickable for all rows, so it's just a single click. For keyboard navigation I understand that it seems bothersome to require multiple key presses, but I think it's part of making the Grid hierarchy properly accessible with the keyboard.

drewharvey commented 3 years ago

Paralell with the double "Enter" press required, you will also need to double Click in order to activate the Edit button (if you are not using keyboard nav)

When using a custom renderer with addColumn, the button is immediately visible and clickable for all rows, so it's just a single click. For keyboard navigation I understand that it seems bothersome to require multiple key presses, but I think it's part of making the Grid hierarchy properly accessible with the keyboard.

That is what I would expect, but the actual behavior requires two clicks.

grid-renderer-two-clicks

drewharvey commented 3 years ago

@sissbruecker do you know if this issue is difficult to fix? I understand the GridPro's main use cases are: inline editing and keyboard navigation. However, real-world apps are going to need a little more flexibility. In my opinion, it is odd that custom editors don't work specifically with dialogs, even if the ux/solution feels conveluted.

The workaround you provided works, but the column implementations can get very complicated, when actual real-world requirements are factored in, and it doesn't feel very maintainable. It also is awkward to have a mixture of GridPro editors used appropriately, and these sort of renderer hacks. I'm struggling to come up with a reusable design and I feel the developers that need to maintain this code will not be happy.

drewharvey commented 3 years ago

Note - ComponentRenderer workaround does not trigger listeners added via grid.addCellEditStartedListener

vaadin / flow-components