Desktop: Accessibility: Improve "change application layout" screen keyboard accessibility #11718
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
personalizedrefrigerator
added
desktop
personalizedrefrigerator
changed the title
Comment on lines
+21
to
+22
| type ReactButtonProps = React.DetailedHTMLProps<React.HTMLAttributes<HTMLButtonElement>, HTMLButtonElement>; | ||
| interface Props extends Omit<ReactButtonProps, 'onClick'> { |
There was a problem hiding this comment.
Most of the changes to the Button component are to pass unused props through to the <button> component it wraps. This removes the need to enumerate all aria-* props that can be used with custom <Button>s.
|
|
||
| const viewInfo = pluginUtils.viewInfoByViewId(plugins, key); | ||
| if (viewInfo) { | ||
| return PluginService.instance().safePluginNameById(viewInfo.plugin.id); |
There was a problem hiding this comment.
Suggested change
| return PluginService.instance().safePluginNameById(viewInfo.plugin.id); | |
| return PluginService.instance().pluginNameById(viewInfo.plugin.id); |
The above is currently used to label panels from plugins. The "safe" in "safePluginNameById" was intended to mean that the method doesn't throw if the plugin isn't installed/loaded. However, just pluginNameById may make more sense.
Comment on lines
34
to
+44
| &.-fullscreen { | ||
| max-width: 100vw; | ||
| max-height: 100vh; | ||
|
|
||
| &::backdrop { | ||
| background-color: var(--joplin-background-color); | ||
| } | ||
|
|
||
| > .content { | ||
| width: calc(100% - 20px); | ||
| padding: 10px; | ||
| margin: 0; | ||
| padding: 0; |
There was a problem hiding this comment.
These changes allow the "change application layout" screen to completely overlay the main app content. At present,
- The application panels are shown behind the application layout
<dialog>. The modal<dialog>prevents these panels from receiving keyboard focus while changing the application layout. - The layout
<dialog>contains the movement controls and a resizable region for each panel.
pedr
approved these changes
Jan 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request improves the accessibility of the "change application layout" screen. In particular, it,
<dialog>, which causes Electron to prevent keyboard focus from interacting with content behind the "change application layout" screen while open.Note: This pull request refactors parts of
ResizableLayoutto improve type safety.Related to #10795.
To-do
For this pull request:
For a follow-up pull request:
Testing plan
This pull request adds automated end-to-end tests that verify: 1) the change layout screen opens and passes the automated accessibility scanner and 2) clicking "move right" for the sidebar keeps the "move right" button focused.
Manual testing (Fedora 41):
Use the arrows to move the layout items. Press "Escape" to exit.thenSidebar panel, Move right button.Note list panel, move left button.