Hey @icosamuel, thank you so much for contributing! This is impressive work. We'll have time to get into the details of the implementation as we're reviewing of this further but I wanted to start with some general thoughts.

My hope is that we choose a color that will augment visibility and contrast, and will also please the core team. Any suggestion is welcome!

I think what we should do here is keep the original color for inactive selection but change the behavior so that a list item is treated as being actively selected (i.e. selected+focused) as long as focus remains within the List itself. See FocusContainer and its use in CommitMessage for more information of the behavior I'm talking about.

That matches the macOS behavior.

On Windows we can look into whether we want to bring back the dotted focus outline for the item that currently has keyboard focus or not.

I believe this could also potential have the benefit of solving #2957.

Some features (like the diff pannel, or the "show in your file manager") use only one file-change as their source. For those, I have them set up to get the last selected Item (wich will also be the focused one)

I would want to have this be more explicit. Ideally there's three type of list selection "modes" that we'd want to support here. There's single selection (which should be the default), "range" which would only allow for selecting contiguous ranges and "multi" (for lack of a better word) which allows range and/or arbitrary selection (Cmd/Ctrl). The file list is a good example of where the latter mode works well and while not implemented yet the range selection will be a key piece of the planned comparison work we're doing for 1.2 where you should be able to diff a range of commits but never arbitrary non-contiguous commits.

I'd propose a new "mode" prop for the list component that governs selection logic and dedicated callbacks for the different modes so that consumers can chose to only subscribe to the callback they care about. I.e.

onSelectedRowChanged(index: number): void
onSelectedRangeChanged(start: number, end: number): void
onSelectionChanged(selectedItems: ReadonlyArray<number>): void

Some features (like the diff pannel, or the "show in your file manager") use only one file-change as their source. For those, I have them set up to get the last selected Item (wich will also be the focused one)

As for the UI implications of this, speaking specifically about the Diff view I think we should play around with the idea of having a dedicated view for multi-selections. Ideally we'd show all diffs for the selected files stacked on top of each other but I believe we're unable to do so at the moment due to performance concerns. I think a dedicated view could help out with discoverability though. Here's a super-quick mock of how something like that could look.

All-in, a great start here @icosamuel!

cc @donokuda for visibility and more design thoughts.

As for the UI implications of this, speaking specifically about the Diff view I think we should play around with the idea of having a dedicated view for multi-selections.

What about putting the diffs together in a scroll view?

@niik Thanks for the feedback

I'm afraid that we can't focus on multiple HTMLElements. We would have to make a new css class for this state (or is that what you suggested?). Although, you will see that the TAB behaviour is now looping through selected rows before changing context. On top of that, I find the distinction between selected and focused to be useful for single element operations.

I really like the idea of List modes: "single" | "range" | "multi". This will make the three callbacks facultative, but will provide for a flexible interface.

As for the UI implications of this, speaking specifically about the Diff view I think we should play around with the idea of having a dedicated view for multi-selections.

Since my original goal was to provide multiple selection for commit, discard and ignore, I personally won't get into a diff range view for the time being. That being said, I like the idea of the simple dedicated page and I welcome any help from other contributors for this part!

Cheers!

I'm afraid that we can't focus on multiple HTMLElements. We would have to make a new css class for this state (or is that what you suggested?).

Yup, that's exactly what I'm suggesting. We'd add a '.focus-inside' class to the list whenever keyboard focus resides on, or inside of the list wrapper element. We'd then use that to style the list items (.list.focus-inside .list-item) rather than the .list-item:focus pseudo element.

you will see that the TAB behaviour is now looping through selected rows before changing context

Yeah, I think that's an unintended side-effect of

On top of that, I find the distinction between selected and focused to be useful for single element operations.

I think platform consistency will have to take precedence here. There's precedence on Windows (at least in older versions) for rendering an inset focus outline for the selected+focused item but other than that I believe that all selected items in the list should have the same blue selected background color (and white foreground) for as long as the list itself has focus.

That being said, I like the idea of the simple dedicated page and I welcome any help from other contributors for this part!

That part was more me thinking out loud and trying to figure out what the implications of this will be. I don't think it has to be part of this PR but we'll either feature-flag the multi-selection behavior until we can land that or scope this PR more tightly so that we're only dealing with the list improvements.

As for the UI implications of this, speaking specifically about the Diff view I think we should play around with the idea of having a dedicated view for multi-selections. Ideally we'd show all diffs for the selected files stacked on top of each other but I believe we're unable to do so at the moment due to performance concerns. I think a dedicated view could help out with discoverability though. Here's a super-quick mock of how something like that could look.

💯 I think for the sake of moving this PR forward, we can have a simple view that says "# files selected." We can open a separate issue and start implementing those actions from there.

I noticed that toggling multiple checkboxes only works with the keyboard. Is there an expectation that this should also happen when clicking the checkbox with multiple files selected? Are there any UI patterns on macOS or Windows that could tell us?

I noticed that toggling multiple checkboxes only works with the keyboard. Is there an expectation that this should also happen when clicking the checkbox with multiple files selected? Are there any UI patterns on macOS or Windows that could tell us?

This is a great question. We should definitely look into it. The checkboxes certainly adds some complexity. Without them I'd say that clicking on a single item would revert any range or multi selection to a single item selection. But with the checkboxes it's almost like it's a weird click-through mode.

The only precedence I could think of straight away was Windows File Explorer which does use the behavior you described (except it doesn't support using the space bar for toggling selection).

Apologies for the weird gif. LiceCap on my old PC is not cooperating.

On macOS the Reminders app has a different behavior

@niik @donokuda I don't mind, really. Just tell me which behaviour you prefer ;) at the moment it selects one row and then toggles its state

Let's go with selecting one row and toggling its state (as in, how it works right now.) Currently hitting spacebar when multiple files are selected is closer to simultaneously clicking on every checkbox. Given how each platform does it differently, I think that it's okay to keeping it to how it currently works.

I also don't anticipate any huge consequences to workflows as a result of going with this direction.

I tested this and it works well, however I spotted one inconsistency which looks to be new behavior.

Reproduce:

• Select 2 of X files and navigate away from Desktop to another app
• Return to Desktop and notice the selected files are now highlighted in grey (expected)
• Click into the Changes area and now the highlighted files are blue
• Try to use the spacebar to check/uncheck the files. Nothing happens.

Expected behavior: I can use space-bar to check/uncheck the files

I see, @tierninho . Thanks for the tests
Looks like this is not new behaviour, but it is now misleading since the rows are shown to be selected, even if the focus in on the list's frame.

We will have to also bind the list's key input event to toggle the selected elements.

Impressive work, @niik! It was interesting to read and I'm grateful for the opportunity to learn from your reviews and changes!

Many people is following the progress of this feature and we are willing to get it on the app asap!

Impressive work, @niik! It was interesting to read and I'm grateful for the opportunity to learn from your reviews and changes!

Thank you @icosamuel. The impressive work is all on your end. This is a fantastic PR and we're all very excited to land it. @shiftkey just did a review of the additions and tweaks that I made so we should be very close to merging now.

Fantastic work guys, a feature that have been wished by many users, has now become a reality.