Fix keyboard navigation in the single-column mail view #6862

rezbyte · 2024-04-15T11:56:17Z

There is still a possible issue where the tab order and the selection via the arrow keys do not align. If you tab to the third row then press down for instance, the forth row will not be selected.

I am not sure whether to fix this as the arrow keys and tab do slightly different things. The arrow keys focus and open a row while the tab just focuses a row.

Closes #5361.

There is still a possible issue where the tab order and the selection via the arrow keys do not align. If you tab to the third row then press down for instance, the forth row will not be selected. I am not sure whether to fix this as the arrow keys and tab do slightly different things. The arrow keys focus and open a row while the tab just focuses a row.

charlag · 2024-04-15T14:09:09Z

src/gui/base/List.ts

@@ -417,6 +415,10 @@ export class List<T, VH extends ViewHolder<T>> implements ClassComponent<ListAtt
 				row.domElement.style.transform = `translateY(${row.top}px)`
 				row.row.update(item, attrs.state.selectedItems.has(item), attrs.state.inMultiselect)
 			}
+			// Focus the selected row so it can receive keyboard events


I am concerned that we can't just grab focus here. What if user is in another part of the app but the list get updated?
I think we would need to check if focused element is inside of this list

That is a good point. However if we only focus the selected element when the focus is on another item, I suspect this will break tab navigation as it will continually jump to the selected row on tab. I will investigate better places to focus it.

I am stumped on finding a way to fix this without refactoring the List's code to allow the ListModel to get the row's element from List.

That wouldn't be desirable.

I am not sure I understand the concern you expressed above. To me the algorithm is "focus the list row programmatically only when the focus is already inside the list". This doesn't change anything about manual tab navigation I think? Am I missing something?

Ah, I thought programmatically focusing an element affected the tab index. My bad, doing a quick test shows otherwise.

The other issue is that requiring the focus to be inside the list row will change the behaviour in the other layouts. In master, pressing up and down anywhere regardless of focus in the double and three column views will change the selected row. This occurs when I tried only focusing when another row is focused via getting the tag name of the focused element.

that's a good thing to think about but I think it still works.

Say you are in three-column layout. You press up/down.

If your focus was in the list, it will move to another position, which is good

If your focus was not in the list, it will just do the selection, without changing your focus, which is also good

If you are happy with this behaviour outside the single-column layout then let's go for it. The issue's steps do not mention focusing the list for single-column so it would be best not to require it. I have pushed a compromise between the two now.

thank you, I'll try to get back to it soon but I I want to test and get a feel for it first

No worries, thanks for taking the time to review it. 🙂

charlag

these are very good issues to fix but I only had a cursory look for now, I would like to look at it again and also test some more but I'm excited about improving it!

src/gui/SelectableRowContainer.ts

rezbyte · 2024-04-16T12:59:46Z

these are very good issues to fix but I only had a cursory look for now, I would like to look at it again and also test some more but I'm excited about improving it!

Thanks for taking a look into it. It is nice to hear that you wanted this fixed. I will address the issues you have mentioned so far. If you are planning to work on it, let me know so we do it together or at least not double up on work.

As requested during PR review.

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

charlag · 2024-04-22T11:34:45Z

I have found following issues:

If I am in the following situation (opened email is the higher one, tab focus is on the lower one) in 3 column layout, tabbing twice will select something that I don't see and then will loop selection from the other selected element. I cannot use "tab" to select the third column, nor can I use it to select items below
~~Sometimes tabbing back into the list just throws me through random places in the list and I can't even see what is selected (still 3 column layout)~~ this isn't new but is related
sometimes selection indicator for the whole list shows up. I can't figure out when exactly, sorry
~~multiselect cannot really be done with keyboard~~ also another issue that's already there

It it better in that it doesn't open the third column halfway anymore though!

charlag · 2024-04-22T11:38:23Z

I am not sure how to test calendar, I cannot focus an agenda item with keyboard at all right now

rezbyte · 2024-04-22T11:47:57Z

I am not sure how to test calendar, I cannot focus an agenda item with keyboard at all right now

I believe the agenda view uses it's own component (CalendarAgendaItemView) instead of the list component so it should not be affected by this PR. Perhaps, it is worth making it use the list component?

charlag · 2024-04-22T16:04:36Z

Oh, my bad, I tested calendar because I thought I was you mentioned it somewhere but I probably mixed it up

rezbyte · 2024-04-25T14:08:08Z

* If I am in the following situation (opened email is the higher one, tab focus is on the lower one) in 3 column layout, tabbing twice will select something that I don't see and then will loop selection from the other selected element. I cannot use "tab" to select the third column, nor can I use it to select items below

I have just pushed a commit that fixes this and should be a better solution for the focus stealing issue.

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

charlag · 2024-05-13T10:07:49Z

I've tested and I think your fix helps!

I figured when/why our focus goes haywire.
Since it's a virtual list we recycle the DOM elements and we rearrange them semi-arbitrary using transform, we would need to manipulate the focus/tab order somehow to fix it but this is out of scope for now I guess

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

rezbyte added 5 commits April 12, 2024 16:43

Fix being unable to navigate lists via keyboard in single column mode

d84e1cb

Closes #5361.

Fix tabbing into the mail column from the list column on single column

66a04ae

Fix the hidden checkbox being in the tab index on mobile

bf74f38

Fix the selected list row being dehighlighted on tab

da299db

charlag reviewed Apr 15, 2024

View reviewed changes

src/gui/SelectableRowContainer.ts Outdated Show resolved Hide resolved

rezbyte added 2 commits April 16, 2024 16:09

Rename SelectableRowContainer.isUsingKeyboard()

7400803

As requested during PR review.

Prevent an updated list stealing the focus outside single-column layout

2a21aae

rezbyte added a commit that referenced this pull request Apr 19, 2024

Allow focusing the calendar events

83547b9

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

Make the list focus only the newly selected event

50230f2

wrdhub pushed a commit that referenced this pull request May 8, 2024

Allow focusing the calendar events

a84cbc2

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

charlag approved these changes May 13, 2024

View reviewed changes

wrdhub added this pull request to the merge queue May 22, 2024

Merged via the queue into master with commit e8085bd May 22, 2024
1 check passed

wrdhub deleted the list-keyboard-controls-5361 branch May 22, 2024 08:46

wrdhub pushed a commit that referenced this pull request May 22, 2024

Allow focusing the calendar events

f06a3c8

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

github-merge-queue bot pushed a commit that referenced this pull request May 22, 2024

Allow focusing the calendar events

537fc6e

There is a bug where you can tab into another `ViewColumn` but this is fixed in an existing PR, #6862. (commit 66a04ae)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix keyboard navigation in the single-column mail view #6862

Fix keyboard navigation in the single-column mail view #6862

rezbyte commented Apr 15, 2024

charlag Apr 15, 2024

rezbyte Apr 16, 2024

rezbyte Apr 17, 2024 •

edited

charlag Apr 18, 2024

rezbyte Apr 18, 2024

charlag Apr 19, 2024

rezbyte Apr 19, 2024 •

edited

charlag Apr 19, 2024

rezbyte Apr 19, 2024

charlag left a comment

rezbyte commented Apr 16, 2024

charlag commented Apr 22, 2024

charlag commented Apr 22, 2024

rezbyte commented Apr 22, 2024

charlag commented Apr 22, 2024

rezbyte commented Apr 25, 2024

charlag commented May 13, 2024

Fix keyboard navigation in the single-column mail view #6862

Fix keyboard navigation in the single-column mail view #6862

Conversation

rezbyte commented Apr 15, 2024

charlag Apr 15, 2024

Choose a reason for hiding this comment

rezbyte Apr 16, 2024

Choose a reason for hiding this comment

rezbyte Apr 17, 2024 • edited

Choose a reason for hiding this comment

charlag Apr 18, 2024

Choose a reason for hiding this comment

rezbyte Apr 18, 2024

Choose a reason for hiding this comment

charlag Apr 19, 2024

Choose a reason for hiding this comment

rezbyte Apr 19, 2024 • edited

Choose a reason for hiding this comment

charlag Apr 19, 2024

Choose a reason for hiding this comment

rezbyte Apr 19, 2024

Choose a reason for hiding this comment

charlag left a comment

Choose a reason for hiding this comment

rezbyte commented Apr 16, 2024

charlag commented Apr 22, 2024

charlag commented Apr 22, 2024

rezbyte commented Apr 22, 2024

charlag commented Apr 22, 2024

rezbyte commented Apr 25, 2024

charlag commented May 13, 2024

rezbyte Apr 17, 2024 •

edited

rezbyte Apr 19, 2024 •

edited