Fix: only focus editor if search field is not focused #2531

codebykat · 2020-12-17T04:51:20Z

Fix

Fixes #2530

This one is a bit hard to reproduce as it's a race condition, but it happens fairly frequently to me on local dev.

Also Fixes #2613

If you search for a term and toggle through the results and then search again the focus is brought to the editor.

Test

Load the app and quickly click into the search field and start typing a search
You should never end up in the editor window accidentally typing in your note
If the editor has focus it should keep focus as you switch notes, create and delete notes, etc.
Perform a search
Use shortcut keys to toggle through search results (CmdOrCtrl + g)
Go back to search again (CmdOrCtrl + f)
Type a new search term
Focus should stay in the search element

Release Notes

Fixed a couple of bugs where the editor would get focus instead of staying with the search field.

belcherj · 2021-01-08T15:55:04Z

Have search query
Select a new note
Shouldn't it focus the note editor?

codebykat · 2021-01-08T21:21:37Z

@belcherj I updated this to directly check document.activeElement, which is not very React-y, but it's pretty hard to use a ref for a sibling element.

I'm not sold about triggering focus() on a note change honestly, though this now handles that case (which is to say, it focuses the editor text on a note switch). It seems like if your cursor is in the search field, and you're filtering notes, you might expect it to stay there (so you could refine your filtering after checking to see which notes are in the results). At any rate this now behaves in line with the current behavior, minus the race condition bug.

dmsnell · 2021-01-08T23:27:43Z

lib/note-content-editor.tsx

-    this.focusEditor();
+    if (document?.activeElement?.id !== 'search-field') {
+      this.focusEditor();
+    }


there are many ways this can go wrong, far more than the one way it can go right.

we should maybe formalize the rules for changing focus and then see if a more elegant solution drops out. maybe there's some global (Redux) state value tracking what context we're in that changes when we enter or blur the search field…

maybe we only care about the initial load? that is, we should grab this.focusEditor() unless the search bar has already been focused, something we could grab from Redux or from a window-global value - a latch that only turns on the first time the search field is focused.

Okay, I will revisit. I agree that we should change the focus as little as possible. It's pretty annoying when the editor fails to get focus, but we really only want to force-focus the editor if there's not another input element focused (in practice this means the search field, although I suppose it might be interesting to keep focus on the tag bar through a note selection change).

That being said:

there are many ways this can go wrong,

I'm curious if you found any bugs with this implementation or know of a case where checking the activeElement would fail.

I'm not sure that we only care about initial load, as @belcherj pointed out, right now it also refocuses the editor after you change notes.

it's not about activeElement as much as it's !== 'search-field' since it's the kind of thing that can end up feeling like spooky action at a distance when we start working with focus on something else, like sort direction or the tag list.

I didn't try this out and I didn't find or see an immediate error with it; just noting that this could be hard to find when the next PR dealing with focus comes around.

to my understanding, re-focusing the editor after changing notes has been intentional since you might want to be opening up a note and then start typing. they keyboard shortcuts still allow you to navigate the note list even when the focus is being taken.

is it only on boot or is it on every note change we care about preserving focus? is there some kind of even that "resets" focus and then the first time to get it keeps it? it sounds as we talk like we might be wanting a global focus latch with a first-come-first-keep rule about it. I'm definitely not sure of this of course.

dcalhoun

I wanted provide the context that once #2707 merges, the current aggressive focus capture of the editor also creates an issue for the History Revision Selector.

Specifically, opening and closing the Revision Selector (or any dialog) should move focus back to the previously focused element, which may or may not be the editor. If one is using a keyboard or screen reader to open the Revision Selector, the previously focused element will be the button to trigger opening the Revision Selector.

I chose not to address this issue in #2707, as I perceive it to be a bit complex. I do plan on creating a new Issue to capture the details.

I'm not sure if the issue I outlined should be addressed within this PR, but figured another example of focus competition could prove beneficial for planning the approach used in this PR.

sandymcfadden · 2021-04-14T19:07:33Z

lib/note-content-editor.tsx

@@ -1188,7 +1190,7 @@ const mapStateToProps: S.MapState<StateProps> = (state) => ({
  note: state.data.notes.get(state.ui.openedNote),
  notes: state.data.notes,
  searchQuery: state.ui.searchQuery,
-  selectedSearchMatchIndex: state.ui.selectedSearchMatchIndex,
+  selectedSearchMatchIndex: state.ui.selectedSearchMatchIndex ?? 0,


Setting the default to 0 as when we were comparing indexes in componentDidUpdate the initial render was comparing null to 0 which was unexpected.

sandymcfadden · 2021-04-14T19:09:05Z

lib/note-content-editor.tsx

-      prevProps.selectedSearchMatchIndex !== this.props.selectedSearchMatchIndex
+      prevProps.selectedSearchMatchIndex !==
+        this.props.selectedSearchMatchIndex &&
+      prevProps.noteId !== this.props.noteId


If you performed a search then toggled through the search results and then performed a new search the focus would come back to the editor instead of finishing typing the search term.
With help from @codebykat I believe this was only intended for when a new note is selected so I added the additional check.

This logic is for deciding when to reset the search selection, isn't it? What does it have to do with the editor focus?

I think the search selection should be updated whenever the search match index changes, not only when changing to a different note.

The actual problem here, which is perhaps being masked, seems to be that setSearchSelection makes another call to this.focusEditor();. That seems like something it should not do?

You're right that is what this does. At first I didn't see any other time than note switches where this would be needed, but that was naive and I did of course find other times.

You're right again that the problem stems from the call to this.focusEditor(); in setSearchSelection. I had first thought to remove that all together but I'm sure especially if using the keyboard shortcuts to toggle through search matches you would want the focus to end up there so you could immediately edit the text.

I've changed my approach to this now. Instead of calling this.focusEditor(); in setSearchSelection I've moved it to set focus in setNextSearchSelection and setPrevSearchSelection.

The only weird thing I've seen so far with this is that now the behavior is different if you are using the keyboard shortcuts to toggle through search results or if you are using the search results bar at the bottom to toggle through. If using the search results bar you don't get the focus anymore. The visual selection is still there though.

I think this may be OK as if you are using the keyboard shortcuts you wouldn't want to then have to click to edit, but if you are clicking the next and previous buttons it would likely be OK to click to where you want to edit.

Would love others thoughts on that part as well.

This makes sense and I agree with you - I think leaving the focus on the buttons is best when using the buttons.

…e search selection

codebykat

So I admit I'm deeply confused by the state of this PR, maybe because I don't remember the context surrounding it. But I just can't follow why this should solve the problem, if it does in fact solve the problem.

Why do we need to focus the editor within setSearchSelection?

Why would we want to avoid updating the search match highlights when the selected search index changes, regardless of whether we have changed notes?

@sandymcfadden I'm sorry if I led you astray here 😅

codebykat · 2021-04-16T07:53:13Z

lib/note-content-editor.tsx

-      prevProps.selectedSearchMatchIndex !== this.props.selectedSearchMatchIndex
+      prevProps.selectedSearchMatchIndex !==
+        this.props.selectedSearchMatchIndex &&
+      prevProps.noteId !== this.props.noteId


This logic is for deciding when to reset the search selection, isn't it? What does it have to do with the editor focus?

I think the search selection should be updated whenever the search match index changes, not only when changing to a different note.

The actual problem here, which is perhaps being masked, seems to be that setSearchSelection makes another call to this.focusEditor();. That seems like something it should not do?

codebykat · 2021-04-16T07:53:20Z

lib/note-content-editor.tsx

-    if (document?.activeElement?.id !== 'search-field') {
-      this.focusEditor();
-    }
+    this.focusEditor();


Doesn't this go back to focusing the editor on load without any checks?

It does, but I don't think this check is necessary. In most cases, I think we want the editor to have focus on load. We just don't want it to unexpectedly take focus if we've clicked the search field.
I believe the focus was getting taken not from the initial on load, but the logic check to see if the selected search index had changed was always executing on first load because it was comparing null to 0 for the prevProps.selectedSearchMatchIndex and props.selectedSearchMatchIndex.

I could be wrong about this as I was having a hard time replicating the focus loss as it was happening to you.

I've resolved that by setting the default to be null. That fixed another bug as well where the first time you pressed CtrlOrCmd + g to go through the search results it would select the second instance instead of the first.

sandymcfadden · 2021-04-16T13:53:25Z

So I admit I'm deeply confused by the state of this PR, maybe because I don't remember the context surrounding it. But I just can't follow why this should solve the problem, if it does in fact solve the problem.

There were certainly some things I was missing here and I apologize for the confusion. I've updated things now and I think addressed your questions in the other comments.

…lection index to be null

codebykat · 2021-04-30T21:40:48Z

This is testing well for me, I can't reproduce the focus issue with searches, changing to different notes, reloading and quickly searching, etc. I say we and see if anything else arises.

✅ approved (geez I wish GitHub allowed us to change author of a PR)

codebykat requested a review from a team December 17, 2020 04:51

codebykat self-assigned this Dec 17, 2020

codebykat added this to the 2.5.0 milestone Dec 17, 2020

codebykat requested a review from illusaen January 7, 2021 23:26

codebykat force-pushed the fix/editor-stealing-focus-on-load branch from 8f6ed5d to 80510a7 Compare January 8, 2021 20:13

codebykat changed the title ~~Fix: only focus editor if search query is empty~~ Fix: only focus editor if search field is not focused Jan 8, 2021

dmsnell reviewed Jan 8, 2021

View reviewed changes

codebykat modified the milestones: 2.5.0, 2.6.0 Jan 9, 2021

codebykat marked this pull request as draft January 9, 2021 23:18

codebykat mentioned this pull request Jan 27, 2021

Keyboard controls #2599

Open

11 tasks

codebykat modified the milestones: 2.6.0 ❄, 2.8.0 Feb 9, 2021

dcalhoun reviewed Mar 9, 2021

View reviewed changes

codebykat modified the milestones: 2.8.0, 2.9.0 Mar 10, 2021

sandymcfadden modified the milestones: 2.9.0, 2.10.0 Mar 20, 2021

dcalhoun mentioned this pull request Mar 23, 2021

Opening and dismissing Revision Selector with screen reader/keyboard does not return focus to previously focused element #2802

Open

sandymcfadden reviewed Apr 14, 2021

View reviewed changes

codebykat and others added 3 commits April 15, 2021 05:41

only focus editor if search query is empty

d1ca704

check if search input is active element rather than if there's a query

0855fb7

Update check to see if a new note has been selected before setting th…

1dbfddf

…e search selection

sandymcfadden force-pushed the fix/editor-stealing-focus-on-load branch from b2f31ee to 1dbfddf Compare April 15, 2021 08:53

sandymcfadden assigned sandymcfadden and unassigned codebykat Apr 15, 2021

sandymcfadden requested a review from a team April 15, 2021 10:49

sandymcfadden removed the request for review from illusaen April 15, 2021 10:49

sandymcfadden marked this pull request as ready for review April 15, 2021 10:52

codebykat commented Apr 16, 2021

View reviewed changes

Update where focus is called for the editor and set default search se…

097ee2b

…lection index to be null

sandymcfadden modified the milestones: 2.10.0, 2.11.0 Apr 18, 2021

sandymcfadden merged commit af85499 into develop May 1, 2021

sandymcfadden deleted the fix/editor-stealing-focus-on-load branch May 1, 2021 11:50

snyk-bot mentioned this pull request Jan 18, 2022

[Snyk] Fix for 1 vulnerabilities aliceUnhinged613/simplenote-electron#20

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: only focus editor if search field is not focused #2531

Fix: only focus editor if search field is not focused #2531

codebykat commented Dec 17, 2020 •

edited by sandymcfadden

Loading

belcherj commented Jan 8, 2021

codebykat commented Jan 8, 2021

dmsnell Jan 8, 2021

codebykat Jan 9, 2021

codebykat Jan 9, 2021

dmsnell Jan 10, 2021

dcalhoun left a comment

sandymcfadden Apr 14, 2021

sandymcfadden Apr 14, 2021

codebykat Apr 16, 2021

sandymcfadden Apr 16, 2021

codebykat Apr 30, 2021

codebykat left a comment

codebykat Apr 16, 2021

codebykat Apr 16, 2021

sandymcfadden Apr 16, 2021 •

edited

Loading

sandymcfadden commented Apr 16, 2021

codebykat commented Apr 30, 2021

Fix: only focus editor if search field is not focused #2531

Fix: only focus editor if search field is not focused #2531

Conversation

codebykat commented Dec 17, 2020 • edited by sandymcfadden Loading

Fix

Test

Release Notes

belcherj commented Jan 8, 2021

codebykat commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcalhoun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebykat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandymcfadden Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

sandymcfadden commented Apr 16, 2021

codebykat commented Apr 30, 2021

codebykat commented Dec 17, 2020 •

edited by sandymcfadden

Loading

sandymcfadden Apr 16, 2021 •

edited

Loading