vaadin / flow-components

Java counterpart of Vaadin Web Components
100 stars 66 forks source link

Spreadsheet focuses the sheet rather than the component when displaying a cell edit component #6529

Open WoozyG opened 2 months ago

WoozyG commented 2 months ago

Description

When a cell with a custom editor is clicked, the custom editor is displayed instead of (or on top of, hiding) the cell static contents. However, after displaying the editor component, the SheetWidget client-side code sets the focus to the sheet, not the editor.

This line is from the initial code commit 10 years ago, per Git-Blame. To my mind that means there is no institutional knowledge about why that focus choice is there :) I don't remember my 10 year old code, at least.

This causes custom edit component use to require multiple clicks, and even breaks keyboard navigation, as attempting to edit a cell suddenly jumps focus back to the sheet, requiring navigating to the cell once again before the component can be used.

Further, I noticed this focus shift also interferes with a ComboBox used as an edit component with autoselect=true such that once the user finally activates the component the filter contents are no longer selected.

Expected outcome

Clicking a Spreadsheet cell with a custom editor should display and focus the editor.

Minimal reproducible example

define a SpreadsheetComponentFactory subclass and add it to a Spreadsheet component. Implement only getCustomEditorForCell() and return a simple component, in my case a ComboBox, for all cells or a specific cell.

In the browser, click that cell, and note where the focus ends up after one cell click.

Steps to reproduce

In the browser, click that cell, and note where the focus ends up after one cell click when the cell has a custom edit component.

Environment

Vaadin version(s): 24.4.8 OS: N/A

Browsers

Issue is not browser related

rolfsmeds commented 2 months ago

Hi @WoozyG, It seems to me that the described behavior is pretty much in line with how the default editor behaves: a single click on a cell only focuses the cell, and does not place focus in the editor. If a single-click did focus the editor, it seems to me that that would make it rather difficult to focus the cell itself, when that is desired.

Perhaps the problem is that the editor component being revealed on click causes a confusing UX that raises expectations of it also getting focused? (Compared to default cells which don't change in appearance when clicked, that is.)

WoozyG commented 2 months ago

@rolfsmeds excellent point. Our use case doesn't include needing to select a cell with an editor component, those cells always want the editor focused on click, vs. cells with plain text or numbers, for example, which in our use case are locked (non-editable).

Maybe the real question is can we have a way to tell the client implementation what to do when displaying an edit component?

If it isn't visible on first click, not only does it take two clicks, you don't know it needs a second click, as there is no indication it would do anything, especially if other cells are locked and not editable.

Perhaps I could also look at always displaying the component, as cell contents, rather than just as an edit component. We chose not to do that originally (7 years ago) as the guidance was to not show too many components so it didn't bog down the UI. However for us, we only have a few per sheet, and use them as dynamic filter controls to let users change what data is displayed in tables and graphs.

Simplest from a code change on my end viewpoint is an API enhancement to allow the backend code that generates/returns the edit component to specify if it receives focus immediately or if the containing cell does.

More involved would be rewriting things to display the components always, so a cell click could trigger an event that focused the component, although even in that case it might be nice to have a way to tell the UI that's the behavior we want, to avoid the delay of a round-trip.

rolfsmeds commented 2 months ago

It seems to me that two behaviors would be reasonable options:

Option A would be consistent with default cells/editors, while option B would be consistent with how fields and other components work outside of the Spreadsheet.

The current behavior, where custom editors become visible on cell focus, and focused (or otherwise activated) with a single-click seems like a strange compromise between the two that is consistent with absolutely nothing. To make things worse, double-clicking a cell with a custom editor that isn't already focused, or pressing Enter/F2 on a focused custom-editor cell, do nothing.

The problem is of course that changing the default to A or B is a behavior altering change, but personally I think it's small enough that it would be ok to introduce it in a minor version, especially considering that the current behavior is certainly a design flaw if not an outright bug.

Out of the two behaviors, perhaps option B would run the lesser risk of confusing/angering existing users used to the current behavior?

In any case, this would ideally then be together with

WoozyG commented 2 months ago

I like option B for our purposes, don't know what other folks might use custom editors for, but for us, they are not for table data edits but for formula control parameters, so only a few per sheet, and having them always visible seems like a good improvement in usability. Plus these would less often be cells folks would want to select in ranges, and if they did, they could start from a different cell and drag.

rolfsmeds commented 2 months ago

Ok, we've decided to make this change, it's in our backlog, we deem the change in behavior small enough to be acceptable in a minor version, but as it's more of refactor+enhancement, it's not considered a bug.

WoozyG commented 2 months ago

Great, thanks for the update. I'll stop toying with it and work on other final bugs in my alpha :)

DiegoCardoso commented 19 hours ago

My first attempt to prototype this was to add a check for null after the call getCustomComponentForCell here to then call getCustomEditorForCell, so it would be a "fallback". https://github.com/vaadin/flow-components/blob/e02d28a7a63337e98fe48758935b0516d821286e/vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/Spreadsheet.java#L4557-L4559

I also had to do some other changes in the client code, but the editor was being displayed. However there were some nasty behavior that needs checking where, after interacting with a ComboBox, the element was removed and focusing on the cell again wouldn't make it show again. I published the proto on this the proto/spreadsheet/custom-editor branch.