Skip to content
This repository has been archived by the owner on Dec 12, 2022. It is now read-only.

Can’t click a button inside a column using Space #122

Closed
jouni opened this issue Mar 15, 2018 · 18 comments
Closed

Can’t click a button inside a column using Space #122

jouni opened this issue Mar 15, 2018 · 18 comments
Assignees
Milestone

Comments

@jouni
Copy link
Member

jouni commented Mar 15, 2018

A <vaadin-button> inside a column can’t be clicked with the keyboard Space when the cell is focused.

This is reproducible in BeverageBuddy “Categories” view.

screen shot 2018-03-15 at 12 15 48

I’m expecting that <flow-grid-component-renderer> causes this – <vaadin-grid> doesn’t consider nested buttons/focusable elements when triggering the click. Therefore it might be that this needs a fix in the web component and not in the Flow wrapper.

See https://vaadin.com/components/vaadin-grid/html-examples#grid-selection-demos (last example) for reference how it should work.

@platosha can probably answer right away?

@pleku
Copy link
Contributor

pleku commented Mar 15, 2018

I’m expecting that causes this – doesn’t consider nested buttons/focusable elements when triggering the click.

This might be the root cause for https://github.com/vaadin/vaadin-grid-flow/issues/74 also.

@pleku pleku added the bug label Mar 15, 2018
@platosha
Copy link

I’m expecting that causes this – doesn’t consider nested buttons/focusable elements when triggering the click.

True: https://github.com/vaadin/vaadin-grid/blob/9f466f3547f86298b029c0a6eea6947ea5bbe52c/src/vaadin-grid-keyboard-navigation-mixin.html#L419

@bennewi
Copy link

bennewi commented May 29, 2018

Similar to if you use a keyboard to navigate to a cell in a Grid that contains a TextField, you can't enter data in the TextField

@tomivirkki
Copy link
Member

<vaadin-grid> invokes click() on the first child element inside the focused cell on space press as a convenience.

To fix this issue (caused by wrapping of the content), listen to click events on <flow-grid-component-renderer> element and in the listener, invoke click() on its first child element respectively.

@bennewi
Copy link

bennewi commented May 29, 2018

Is it not better, if the cell gets focus, that an evaluation is made if the cell has a component renderer and if so, immediately pass the focus to the embedded component? This way you wouldn't be treating "space" and "click" as separate cases but you could interact with any component as though it were the cell. My case of using a TextField to enable editable table data isn't linked to a user pressing space...

@tomivirkki
Copy link
Member

@bennewi I think we're talking about a different things here. This ticket is about the "space press click" feature. Focusing an element inside a cell is a different thing and done with Enter key (by entering the interaction mode).

@jouni
Copy link
Member Author

jouni commented May 29, 2018

an evaluation is made if the cell has a component renderer and if so, immediately pass the focus to the embedded component

The web component doesn’t have a concept of a “component renderer”. AFAIK that’s a Flow concept. I don’t think we should add workarounds in the web component on top of Flow’s workarounds. Edit: Unless this is a generic problem, and there’s a way to fix that.

@bennewi
Copy link

bennewi commented May 29, 2018

@tomivirkki was this changed recently? When I have a Grid cell selected, pressing enter doesn't get my cursor into the embedded TextField so I can start typing.

10.0.0.beta9 btw

@tomivirkki
Copy link
Member

@bennewi In case the focusable element is wrapped (like I assume in this case), you can add focus-target attribute to the element to get it focused on enter.

@bennewi
Copy link

bennewi commented May 29, 2018

@tomivirkki Should focus-target not be the default behavior always? What is the usefulness of pressing enter and losing all focus with the Grid, Cell, and Cell contents all at once?

Problem is if you press enter once to leave, since you lost focus with everything you can't even press enter a second time to get back in

@tomivirkki
Copy link
Member

@bennewi It's focusing the first child element by default (if a focus-target isn't defined). Again, this discussion should happen somewhere else because it's about another topic.

@denis-anisimov
Copy link

I cannot reproduce it with the recent beverage.
But I made an IT test for this testcase.

@jouni
Copy link
Member Author

jouni commented Jun 7, 2018

Still reproducible, in Chrome, Safari and Firefox on macOS.

Are you sure you tested the Categories list, and not the Reviews list? The reviews list is not using vaadin-grid.

In the Categories list, use the Tab key to move the focus to the grid body. Then use the arrow keys to move the focus to the cell which contains the Edit button. Then press Space. Nothing happens.

The test you added doesn’t really test this behavior, as you are sending the Space key event directly to the button element, not for the cell element.

@denis-anisimov
Copy link

True, I've used reviews list.
Yes, I can reproduce it now with categories list.
Not sure that its possible to write the test for this at all. But at least will try to fix the issue.

@jouni
Copy link
Member Author

jouni commented Jun 8, 2018

Was the issue actually fixed, or did you just add the test for it?

@ZheSun88
Copy link
Contributor

ZheSun88 commented Jun 8, 2018

the fix is from flow side, PR vaadin/flow#4244 has been merged

@ZheSun88
Copy link
Contributor

The current fix has been reverted, because of the side effects on overlay component

@denis-anisimov
Copy link

The test is disabled : #226
Should be enabled back once the fix is provided.
The original fix is reverted : vaadin/flow#4269.
The fix cannot be done in this way.

denis-anisimov pushed a commit that referenced this issue Jun 12, 2018
denis-anisimov pushed a commit that referenced this issue Jun 26, 2018
caalador pushed a commit that referenced this issue Jun 26, 2018
ZheSun88 pushed a commit that referenced this issue Jul 16, 2018
ZheSun88 pushed a commit that referenced this issue Jul 17, 2018
ZheSun88 pushed a commit that referenced this issue Jul 17, 2018
ZheSun88 pushed a commit that referenced this issue Jul 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants