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

Desktop: Fix editor/viewer loses focus when visible panels are changed with ctrl-l #11029

Merged

Conversation

personalizedrefrigerator
Copy link
Collaborator

Summary

This pull request fixes an issue originally reported on the forum — focusing the editor, then pressing ctrl-l to cycle through the different editor/viewer layouts causes focus to be lost.

This pull request causes the toggleEditorLayout command to refocus the editor or viewer such that the note editor retains focus on layout change. In particular,

  • If the editor has focus and is hidden, focus moves to the viewer.
  • If the editor is shown and the viewer had focus, moves focus to the editor.
  • If neither the editor nor the viewer has focus, the focus is not changed.
    • This might happen if, for example, the user activated the "change editor layout button" with a keyboard.

Testing plan

This pull request includes an automated test. It has also been tested manually on Fedora 40 using the following steps:

  1. Focus the note editor.
  2. Press ctrl-l. Verify that the viewer is hidden.
  3. Verify that the note editor still has focus.
  4. Press ctrl-l. Verify that only the viewer is visible.
  5. Verify that the note viewer has focus (that arrow keys scroll the note viewer).
  6. Press ctrl-l.
  7. Verify that the note editor has focus.
  8. Focus the title, then navigate to the "toggle editor layout" button with the tab and arrow keys.
  9. Press enter.
  10. Verify that the visible panes changed, but the "toggle editor layout" button still has focus.

@@ -132,9 +131,14 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
public focus() {
if (this.webviewRef_.current) {
focus('NoteTextViewer::focus', this.webviewRef_.current);
this.send('focus');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, after .focusing the viewer, the arrow keys didn't scroll the viewer's content.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you this comment in the code? I'm not a big fan of having two different ways to focus something but if it's an exception we can leave it but with a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original focus('NoteTextViewer::focus', this.webviewRef_.current); seems to be unnecessary — I've removed it (leaving this.send('focus')) and added a comment. Edit: Tests fail when focus(..., webviewRef) is removed. Additionally, without it, .focusing the viewer doesn't seem to work when not triggered by user interaction.

@personalizedrefrigerator personalizedrefrigerator changed the title Desktop: Fix editor/viewer lose focus when visible panels are changed with ctrl-l Desktop: Fix editor/viewer loses focus when visible panels are changed with ctrl-l Sep 11, 2024
@personalizedrefrigerator personalizedrefrigerator added accessibility Related to accessibility desktop All desktop platforms labels Sep 11, 2024
@laurent22 laurent22 merged commit bcb5218 into laurent22:dev Sep 12, 2024
10 checks passed
@personalizedrefrigerator
Copy link
Collaborator Author

Related to #10795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Related to accessibility desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants