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

Notebook cell list focus/selection model #117868

Closed
rebornix opened this issue Mar 1, 2021 · 8 comments
Closed

Notebook cell list focus/selection model #117868

rebornix opened this issue Mar 1, 2021 · 8 comments
Assignees
Labels
debt Code quality issues on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@rebornix
Copy link
Member

rebornix commented Mar 1, 2021

The notebook editor is a list widget/view under the hood, thus users can use their familiar list view keybindings to navigate and select cells in the notebook editor. It works well with single selection mode but fell apart after we turned on the multiple selection mode.

Selection/Focus model difference

The multi-selection/focus mode in the builtin list view seems inherits the concept of selection/focus of the File Explorer (VS Code one, not the OS one), and the major difference we are running into is

  • List View
    • Single click on an entry selects and focuses on the entry
    • Arrow up/down will move the focus to previous/next entry and it does not move or expand the selections. Side effect of this is focused items don't necessarily need to be selected items.
  • Notebook
    • Single click on an entry selects and focuses on the entry
    • Arrow up/down will move the focus and selection to previous/next entry
    • Shift+Arrow up/down will expand the selection, and also move the focus to previous/next entry
    • Click+Arrow up/down behaves the same

The list view behavior makes sense for the File Explorer as it's a master-details view, the selected entry might be opened in the editor pane so arrow up/down moves the focus, but not the "selection". However in Notebook, it's simply a list, pretty close to Finder, arrow keys just moves the focus/selection up and down. The only difference between Notebook list and Finder is probably that in Finder, there is no "focus" but in notebook, the lastly activated item is the focused element.

Proposal 1

When @roblourens and me discussed about this, we talked about if we should align completely with the default list view. The catch is it feels weird in notebook list, even for the simplest scenario

  • Click the first cell, it's selected and focused
  • Arrow down, the second cell is focused, the first cell is selected. The first cell has a selection background while the second cell has a focus border

It's beyond my expectation and IMHO users will get confused too. The advantage of this solution is though all list commands and features just work seamlessly, and we don't need to tweak how selections/focus are handled, which will definitely lead to interesting bugs.

Proposal 2

The other way around is keeping our current UX in notebook, but I found that I'm almost fighting against the list view logic (how it setFocus and setSelection):

  • Arrow down, calls setFocus only
  • Shift + Arrow down, calls setFocus and then setSelection

Whether the selection should be updated depends on if a setSelection call is invoked and we can't tell that when setFocus is invoked. Almost every list view navigation/selection related commands leverage setFocus and setSelection to archive the file_explorer_style experience. To completely workaround this in notebook, it seems we have to

  • Implement our own list down/up, expandDown/up, selectDown/up commands, etc
  • For focusFirst, focusLast, focusPageUp, focusPageDown commands, then setFocus is called, make sure the focused element is also selected

The drawback is list commands are no longer useable in Notebook and we may also want to disable list commands completely otherwise users will run into weird state as list commands can call setFocus and setSelection in a wrong order (wrong = not what we expect).


@joaomoreno I'd like to hear your thoughts on this, especially how to approach this if we want a different focus/selection model than the default one. Currently I prefer the second approach but found it challenging to implement.

@rebornix rebornix added the under-discussion Issue is under discussion for relevance, priority, approach label Mar 1, 2021
@rebornix rebornix self-assigned this Mar 1, 2021
@rebornix
Copy link
Member Author

rebornix commented Mar 1, 2021

cc @misolori, @roblourens and @jrieken for inputs.

@joaomoreno
Copy link
Member

One thing I failed to understand from the issue is: what is the expected behavior for Notebooks? That is: what do you expect to happen when pressing all combinations of Ctrl/Shift with arrow keys?

@rebornix
Copy link
Member Author

rebornix commented Mar 1, 2021

That is: what do you expect to happen when pressing all combinations of Ctrl/Shift with arrow keys

@joaomoreno sorry that I didn't make it clear:

  • arrow down, moves the focus/select to next
  • shift arrow down, expands the selection to next, move focus to next

now we don't use ctrl/alt modifier yet so above two are the only major difference and what we expect in a notebook list.

@rebornix rebornix added the debt Code quality issues label Mar 1, 2021
@rebornix rebornix added this to the March 2021 milestone Mar 1, 2021
@roblourens
Copy link
Member

I still think that being consistent with other lists will save us a lot of time and energy. Maybe with minor tweaks we can address things that feel off in notebooks, like

  • Single click by itself doesn't select anything, shift+click or ctrl+click work as they do in other lists
  • Or if no bg color shows up when only a single cell is selected, then your example in 1 is ok, it doesn't matter that one cell is selected

Then there is the question of how this gets exposed in API, we probably don't have to expose the exact details of focus vs selection in API. We can decide what the goal of the API is, design the right API for that, and map it to our internal model.

@rebornix
Copy link
Member Author

rebornix commented Mar 2, 2021

I still think that being consistent with other lists will save us a lot of time and energy

I agree on this, staying close to the list view logic will be the best for engineering efforts. The concern I have is how many workarounds/tweaks we need to take to reach a reasonable state

Single click by itself doesn't select anything, shift+click or ctrl+click work as they do in other lists

It makes ArrowDown work as expected but breaks Shift+ArrowDown. Say we single click on an item A, A is not selected but only focused. Shift+Arrow should then expand selection to B, but there is no selection now, list.expandSelection will now mark B as the selected and focused element. A is left out.

You will not see any cell bgcolors in this case as it's still not multi-select. However from user's perspective, it should be multi-select as they press shift+arrowdown.

I wonder if we can have tweaks which can ensure they all work as expected. But currently I didn't think so considering how list.moveFoucs and list.expandSelection use the two API setFocus/setSelection

@rebornix
Copy link
Member Author

rebornix commented Mar 2, 2021

Feedback we got from the standup today about if we want list commands work seamlessly in notebook land

@mjbvz :
For the notebook commands, I think as long as the keybindings and behavior are the same, we don't need to reuse the list commands
As a user, I don't think of notebooks as being lists

@rebornix
Copy link
Member Author

Already supported via List View's new navigation model.

@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants