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

CellFocusEvent is triggered when dragging a grid row. #2863

Closed
1 of 3 tasks
javier-godoy opened this issue Mar 14, 2022 · 3 comments · Fixed by vaadin/web-components#3587
Closed
1 of 3 tasks

CellFocusEvent is triggered when dragging a grid row. #2863

javier-godoy opened this issue Mar 14, 2022 · 3 comments · Fixed by vaadin/web-components#3587
Assignees

Comments

@javier-godoy
Copy link
Contributor

javier-godoy commented Mar 14, 2022

Description

A CellFocusEvent is triggered when dragging a grid row.

Expected outcome

CellFocusEvent should not be fired.

Actual outcome

CellFocusEvent is fired after the item is dropped and a button is clicked.

Steps to reproduce

  1. Select row A in Grid
  2. Drag row B to the target TextField
  3. Click the button
  4. Observe that the CellFocusListener executes and reports "B" as focused.

image

Minimal reproducible example

    Grid<String> grid = new Grid<>();
    grid.setSelectionMode(SelectionMode.SINGLE);

    add(grid);
    grid.setItems("A", "B", "C");
    grid.addColumn(x -> x).setHeader("Drag item");

    TextField target = new TextField("Drop here");
    add(new Button("Click"));
    add(target);

    grid.addCellFocusListener(ev->{
      ev.getItem().ifPresent(item -> Notification.show("FOCUS: " + item));
    });

    grid.addDragStartListener(event -> {
      draggedItem = event.getDraggedItems().get(0);
    });

    grid.setRowsDraggable(true);

    grid.addDragEndListener(event -> {
      draggedItem = null;
    });

    DropTarget<?> dropTarget = DropTarget.create(target);
    dropTarget.addDropListener(event -> {
      target.setValue(draggedItem);
    });

Environment

  • v14.8.5
  • v23.0.1

Affected Browsers

  • Chrome
  • Safari
  • Firefox
@sissbruecker
Copy link
Contributor

Can reproduce the issue, for v14 as well as v23. Could only reproduce it in Chrome. In Chrome it requires an actual click to focus the cell, in other browsers I tested a mouse down is enough.

That no cell focus event is expected is debatable, but usually a mouse down is enough to focus an element, so I would expect the cell focus to work the same way. In this case, I would expect the cell focus event to trigger as soon as the user starts to drag.

@yuriy-fix yuriy-fix added the BFP label Mar 15, 2022
@sirbris
Copy link

sirbris commented Mar 15, 2022

In this case, I would expect the cell focus event to trigger as soon as the user starts to drag.

It would be important for us to distinguish between a simple click, keyboard navigation and a drag. In our case we use the cellFocusEvent to automatically select that item. Dragging an item should not also select it, so we need some kind of information in the event to know what's going on and adjust our logic accordingly.

@sissbruecker sissbruecker self-assigned this Mar 16, 2022
@sissbruecker
Copy link
Contributor

The currently expected behavior is that the cell-focus event triggers on mouse down, which is how native elements work as well (demo). This is the case for all browsers, except for Chrome, where we use a workaround to delay the cell-focus event to mouse up. Changing this behavior as part of a bugfix is not possible, as it would be a breaking change and might lead to issues for other developers. What we can do is address the problematic behavior in Chrome, where the cell-focus is triggered after clicking somewhere after a drag operation. But again, this would only affect Chrome, for other browsers the cell-focus would still trigger on mouse down.

I can see that the current behavior is problematic when trying to distinguish between dragging and cell-focus. I would suggest to create an enhancement ticket to:

  • either request to change the cell-focus behavior to trigger on click, rather than on mouse down. This could be addressed in a future major version, if there is consent that this is the right choice
  • or add information to the cell-focus event whether it was triggered through mouse or keyboard interaction. This could be addressed in a future minor version

As for workarounds, one solution could be to manually detect whether the user is interacting with the keyboard while processing the the cell-focus event:

        ComponentUtil.addListener(grid, KeyDownEvent.class, e -> {
            keyPressed = true;
        });
        grid.addCellFocusListener(e -> {
            if (keyPressed) {
                // application logic here, for example select item:
                grid.select(e.getItem().get());
            }
        });
        ComponentUtil.addListener(grid, KeyUpEvent.class, e -> {
            keyPressed = false;
        });
        grid.addBlurListener(e -> {
            // Also clear keyPressed flag on blur, this can happen when tabbing out of the component
            keyPressed = false;
        });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants