From 14cc053094815ef3464ca40be063319d7238f284 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Fri, 2 Aug 2024 06:49:15 -0700 Subject: [PATCH] Desktop: Accessibility: Improve settings screen keyboard navigation and screen reader accessibility (#10812) --- .eslintignore | 7 +- .gitignore | 7 +- packages/app-desktop/gui/Button/Button.tsx | 29 +- .../gui/ConfigScreen/ConfigScreen.tsx | 489 +++--------------- .../app-desktop/gui/ConfigScreen/Sidebar.tsx | 65 ++- .../{ => controls}/FontSearch.tsx | 2 + .../controls/SettingComponent.tsx | 381 ++++++++++++++ .../controls/SettingDescription.tsx | 12 + .../ConfigScreen/controls/SettingHeader.tsx | 15 + .../ConfigScreen/controls/SettingLabel.tsx | 16 + .../controls/ToggleAdvancedSettingsButton.tsx | 5 + .../controls/plugins/PluginBox.tsx | 14 +- .../controls/plugins/PluginsStates.tsx | 13 +- .../controls/plugins/SearchPlugins.tsx | 5 +- .../app-desktop/gui/ConfigScreen/style.scss | 2 + .../gui/ConfigScreen/styles/index.scss | 5 + .../styles/setting-description.scss | 9 + .../ConfigScreen/styles/setting-header.scss | 9 + .../ConfigScreen/styles/setting-label.scss | 20 + .../styles/setting-tab-panel.scss | 18 + .../EncryptionConfigScreen.tsx | 15 +- .../gui/lib/ToggleButton/ToggleButton.tsx | 18 + .../integration-tests/main.spec.ts | 31 -- .../integration-tests/models/MainScreen.ts | 2 +- .../models/SettingsScreen.ts | 2 +- .../integration-tests/settings.spec.ts | 96 ++++ packages/app-desktop/main.scss | 2 +- packages/lib/models/Setting.ts | 2 +- packages/lib/theme.ts | 2 + 29 files changed, 795 insertions(+), 498 deletions(-) rename packages/app-desktop/gui/ConfigScreen/{ => controls}/FontSearch.tsx (99%) create mode 100644 packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx create mode 100644 packages/app-desktop/gui/ConfigScreen/controls/SettingDescription.tsx create mode 100644 packages/app-desktop/gui/ConfigScreen/controls/SettingHeader.tsx create mode 100644 packages/app-desktop/gui/ConfigScreen/controls/SettingLabel.tsx create mode 100644 packages/app-desktop/gui/ConfigScreen/styles/index.scss create mode 100644 packages/app-desktop/gui/ConfigScreen/styles/setting-description.scss create mode 100644 packages/app-desktop/gui/ConfigScreen/styles/setting-header.scss create mode 100644 packages/app-desktop/gui/ConfigScreen/styles/setting-label.scss create mode 100644 packages/app-desktop/gui/ConfigScreen/styles/setting-tab-panel.scss create mode 100644 packages/app-desktop/integration-tests/settings.spec.ts diff --git a/.eslintignore b/.eslintignore index 360f34c05..6dd919e2a 100644 --- a/.eslintignore +++ b/.eslintignore @@ -167,9 +167,13 @@ packages/app-desktop/gui/Button/Button.js packages/app-desktop/gui/ClipperConfigScreen.js packages/app-desktop/gui/ConfigScreen/ButtonBar.js packages/app-desktop/gui/ConfigScreen/ConfigScreen.js -packages/app-desktop/gui/ConfigScreen/FontSearch.js packages/app-desktop/gui/ConfigScreen/Sidebar.js +packages/app-desktop/gui/ConfigScreen/controls/FontSearch.js packages/app-desktop/gui/ConfigScreen/controls/MissingPasswordHelpLink.js +packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.js +packages/app-desktop/gui/ConfigScreen/controls/SettingDescription.js +packages/app-desktop/gui/ConfigScreen/controls/SettingHeader.js +packages/app-desktop/gui/ConfigScreen/controls/SettingLabel.js packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.js packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js @@ -463,6 +467,7 @@ packages/app-desktop/integration-tests/models/SettingsScreen.js packages/app-desktop/integration-tests/models/Sidebar.js packages/app-desktop/integration-tests/noteList.spec.js packages/app-desktop/integration-tests/richTextEditor.spec.js +packages/app-desktop/integration-tests/settings.spec.js packages/app-desktop/integration-tests/sidebar.spec.js packages/app-desktop/integration-tests/simpleBackup.spec.js packages/app-desktop/integration-tests/util/activateMainMenuItem.js diff --git a/.gitignore b/.gitignore index e2e3d6b80..b2437b914 100644 --- a/.gitignore +++ b/.gitignore @@ -146,9 +146,13 @@ packages/app-desktop/gui/Button/Button.js packages/app-desktop/gui/ClipperConfigScreen.js packages/app-desktop/gui/ConfigScreen/ButtonBar.js packages/app-desktop/gui/ConfigScreen/ConfigScreen.js -packages/app-desktop/gui/ConfigScreen/FontSearch.js packages/app-desktop/gui/ConfigScreen/Sidebar.js +packages/app-desktop/gui/ConfigScreen/controls/FontSearch.js packages/app-desktop/gui/ConfigScreen/controls/MissingPasswordHelpLink.js +packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.js +packages/app-desktop/gui/ConfigScreen/controls/SettingDescription.js +packages/app-desktop/gui/ConfigScreen/controls/SettingHeader.js +packages/app-desktop/gui/ConfigScreen/controls/SettingLabel.js packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.js packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js @@ -442,6 +446,7 @@ packages/app-desktop/integration-tests/models/SettingsScreen.js packages/app-desktop/integration-tests/models/Sidebar.js packages/app-desktop/integration-tests/noteList.spec.js packages/app-desktop/integration-tests/richTextEditor.spec.js +packages/app-desktop/integration-tests/settings.spec.js packages/app-desktop/integration-tests/sidebar.spec.js packages/app-desktop/integration-tests/simpleBackup.spec.js packages/app-desktop/integration-tests/util/activateMainMenuItem.js diff --git a/packages/app-desktop/gui/Button/Button.tsx b/packages/app-desktop/gui/Button/Button.tsx index 3f228b428..56e248ed3 100644 --- a/packages/app-desktop/gui/Button/Button.tsx +++ b/packages/app-desktop/gui/Button/Button.tsx @@ -36,6 +36,9 @@ interface Props { isSquare?: boolean; iconOnly?: boolean; fontSize?: number; + + 'aria-controls'?: string; + 'aria-expanded'?: string; } const StyledTitle = styled.span` @@ -220,7 +223,14 @@ const Button = React.forwardRef((props: Props, ref: any) => { function renderIcon() { if (!props.iconName) return null; - return ; + return ; } function renderTitle() { @@ -234,7 +244,22 @@ const Button = React.forwardRef((props: Props, ref: any) => { } return ( - + {renderIcon()} {renderTitle()} diff --git a/packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx b/packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx index 3778e00e3..572dfc88c 100644 --- a/packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx +++ b/packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx @@ -1,16 +1,14 @@ import * as React from 'react'; import Sidebar from './Sidebar'; import ButtonBar from './ButtonBar'; -import Button, { ButtonLevel, ButtonSize } from '../Button/Button'; +import Button, { ButtonLevel } from '../Button/Button'; import { _ } from '@joplin/lib/locale'; import bridge from '../../services/bridge'; -import Setting, { AppType, SettingItemSubType, SyncStartupOperation } from '@joplin/lib/models/Setting'; -import control_PluginsStates from './controls/plugins/PluginsStates'; +import Setting, { AppType, SettingValueType, SyncStartupOperation } from '@joplin/lib/models/Setting'; import EncryptionConfigScreen from '../EncryptionConfigScreen/EncryptionConfigScreen'; import { reg } from '@joplin/lib/registry'; const { connect } = require('react-redux'); -const { themeStyle } = require('@joplin/lib/theme'); -import * as pathUtils from '@joplin/lib/path-utils'; +import { themeStyle } from '@joplin/lib/theme'; import SyncTargetRegistry from '@joplin/lib/SyncTargetRegistry'; import * as shared from '@joplin/lib/components/shared/config/config-shared.js'; import ClipperConfigScreen from '../ClipperConfigScreen'; @@ -20,12 +18,8 @@ import ToggleAdvancedSettingsButton from './controls/ToggleAdvancedSettingsButto import shouldShowMissingPasswordWarning from '@joplin/lib/components/shared/config/shouldShowMissingPasswordWarning'; import MacOSMissingPasswordHelpLink from './controls/MissingPasswordHelpLink'; const { KeymapConfigScreen } = require('../KeymapConfig/KeymapConfigScreen'); -import FontSearch from './FontSearch'; +import SettingComponent, { UpdateSettingValueEvent } from './controls/SettingComponent'; -// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied -const settingKeyToControl: any = { - 'plugins.states': control_PluginsStates, -}; interface Font { family: string; @@ -67,9 +61,6 @@ class ConfigScreenComponent extends React.Component { this.onCancelClick = this.onCancelClick.bind(this); this.onSaveClick = this.onSaveClick.bind(this); this.onApplyClick = this.onApplyClick.bind(this); - this.renderLabel = this.renderLabel.bind(this); - this.renderDescription = this.renderDescription.bind(this); - this.renderHeader = this.renderHeader.bind(this); this.handleSettingButton = this.handleSettingButton.bind(this); } @@ -237,7 +228,7 @@ class ConfigScreenComponent extends React.Component { if (syncTargetMd.supportsConfigCheck) { const messages = shared.checkSyncConfigMessages(this); const statusComp = !messages.length ? null : ( -
+
{messages[0]} {messages.length >= 1 ?

{messages[1]}

: null}
@@ -277,12 +268,14 @@ class ConfigScreenComponent extends React.Component { let advancedSettingsButton = null; const advancedSettingsSectionStyle = { display: 'none' }; + const advancedSettingsGroupId = `advanced_settings_${key}`; if (advancedSettingComps.length) { advancedSettingsButton = ( shared.advancedSettingsButton_click(this)} advancedSettingsVisible={this.state.showAdvancedSettings} + aria-controls={advancedSettingsGroupId} /> ); advancedSettingsSectionStyle.display = this.state.showAdvancedSettings ? 'block' : 'none'; @@ -293,425 +286,35 @@ class ConfigScreenComponent extends React.Component { {this.renderSectionDescription(section)}
{settingComps}
{advancedSettingsButton} -
{advancedSettingComps}
+
{advancedSettingComps}
); } - private labelStyle(themeId: number) { - const theme = themeStyle(themeId); - return { ...theme.textStyle, display: 'block', - color: theme.color, - fontSize: theme.fontSize * 1.083333, - fontWeight: 500, - marginBottom: theme.mainPadding / 2 }; - } - - private descriptionStyle(themeId: number) { - const theme = themeStyle(themeId); - return { ...theme.textStyle, color: theme.colorFaded, - fontStyle: 'italic', - maxWidth: '70em', - marginTop: 5 }; - } - - private renderLabel(themeId: number, label: string) { - const labelStyle = this.labelStyle(themeId); - return ( -
- -
- ); - } - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - private renderHeader(themeId: number, label: string, style: any = null) { - const theme = themeStyle(themeId); - - const labelStyle = { ...theme.textStyle, display: 'block', - color: theme.color, - fontSize: theme.fontSize * 1.25, - fontWeight: 500, - marginBottom: theme.mainPadding, - ...style }; - - return ( -
- -
- ); - } - - private renderDescription(themeId: number, description: string) { - return description ?
{description}
: null; - } - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - public settingToComponent(key: string, value: any) { - const theme = themeStyle(this.props.themeId); - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const output: any = null; - - const rowStyle = { - marginBottom: theme.mainPadding * 1.5, - }; - - const labelStyle = this.labelStyle(this.props.themeId); - - const subLabel = { ...labelStyle, display: 'block', - opacity: 0.7, - marginBottom: labelStyle.marginBottom }; - - const checkboxLabelStyle = { ...labelStyle, marginLeft: 8, - display: 'inline', - backgroundColor: 'transparent' }; - - const controlStyle = { - display: 'inline-block', - color: theme.color, - fontFamily: theme.fontFamily, - backgroundColor: theme.backgroundColor, - }; - - const textInputBaseStyle = { ...controlStyle, fontFamily: theme.fontFamily, - border: '1px solid', - padding: '4px 6px', - boxSizing: 'border-box', - borderColor: theme.borderColor4, - borderRadius: 3, - paddingLeft: 6, - paddingRight: 6, - paddingTop: 4, - paddingBottom: 4 }; - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const updateSettingValue = (key: string, value: any) => { - const md = Setting.settingMetadata(key); - if (md.needRestart) { - this.setState({ needRestart: true }); - } - shared.updateSettingValue(this, key, value); - }; - + private onUpdateSettingValue = ({ key, value }: UpdateSettingValueEvent) => { const md = Setting.settingMetadata(key); - - const descriptionText = Setting.keyDescription(key, AppType.Desktop); - const descriptionComp = this.renderDescription(this.props.themeId, descriptionText); - - if (settingKeyToControl[key]) { - const SettingComponent = settingKeyToControl[key]; - const label = md.label ? this.renderLabel(this.props.themeId, md.label()) : null; - return ( -
- {label} - {this.renderDescription(this.props.themeId, md.description ? md.description(AppType.Desktop) : null)} - { - updateSettingValue(key, event.value); - }} - renderLabel={this.renderLabel} - renderDescription={this.renderDescription} - renderHeader={this.renderHeader} - /> -
- ); - } else if (md.isEnum) { - const items = []; - const settingOptions = md.options(); - const array = Setting.enumOptionsToValueLabels(settingOptions, md.optionsOrder ? md.optionsOrder() : [], { - valueKey: 'key', - labelKey: 'label', - }); - - for (let i = 0; i < array.length; i++) { - const e = array[i]; - items.push( - , - ); - } - - const selectStyle = { ...controlStyle, paddingLeft: 6, - paddingRight: 6, - paddingTop: 4, - paddingBottom: 4, - borderColor: theme.borderColor4, - borderRadius: 3 }; - - return ( -
-
- -
- - {descriptionComp} -
- ); - } else if (md.type === Setting.TYPE_BOOL) { - const onCheckboxClick = () => { - updateSettingValue(key, !value); - }; - - const checkboxSize = theme.fontSize * 1.1666666666666; - - // Hack: The {key+value.toString()} is needed as otherwise the checkbox doesn't update when the state changes. - // There's probably a better way to do this but can't figure it out. - - return ( -
-
- { - onCheckboxClick(); - }} - style={{ marginLeft: 0, width: checkboxSize, height: checkboxSize }} - /> - -
- {descriptionComp} -
- ); - } else if (md.type === Setting.TYPE_STRING) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const inputStyle: any = { ...textInputBaseStyle, width: '50%', - minWidth: '20em' }; - const inputType = md.secure === true ? 'password' : 'text'; - - if (md.subType === 'file_path_and_args' || md.subType === 'file_path' || md.subType === 'directory_path') { - inputStyle.marginBottom = subLabel.marginBottom; - - const splitCmd = (cmdString: string) => { - // Normally not necessary but certain plugins found a way to - // set the set the value to "undefined", leading to a crash. - // This is now fixed at the model level but to be sure we - // check here too, to handle any already existing data. - // https://github.com/laurent22/joplin/issues/7621 - if (!cmdString) cmdString = ''; - const path = pathUtils.extractExecutablePath(cmdString); - const args = cmdString.substr(path.length + 1); - return [pathUtils.unquotePath(path), args]; - }; - - const joinCmd = (cmdArray: string[]) => { - if (!cmdArray[0] && !cmdArray[1]) return ''; - let cmdString = pathUtils.quotePath(cmdArray[0]); - if (!cmdString) cmdString = '""'; - if (cmdArray[1]) cmdString += ` ${cmdArray[1]}`; - return cmdString; - }; - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const onPathChange = (event: any) => { - if (md.subType === 'file_path_and_args') { - const cmd = splitCmd(this.state.settings[key]); - cmd[0] = event.target.value; - updateSettingValue(key, joinCmd(cmd)); - } else { - updateSettingValue(key, event.target.value); - } - }; - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const onArgsChange = (event: any) => { - const cmd = splitCmd(this.state.settings[key]); - cmd[1] = event.target.value; - updateSettingValue(key, joinCmd(cmd)); - }; - - const browseButtonClick = async () => { - if (md.subType === 'directory_path') { - const paths = await bridge().showOpenDialog({ - properties: ['openDirectory'], - }); - if (!paths || !paths.length) return; - updateSettingValue(key, paths[0]); - } else { - const paths = await bridge().showOpenDialog(); - if (!paths || !paths.length) return; - - if (md.subType === 'file_path') { - updateSettingValue(key, paths[0]); - } else { - const cmd = splitCmd(this.state.settings[key]); - cmd[0] = paths[0]; - updateSettingValue(key, joinCmd(cmd)); - } - } - }; - - const cmd = splitCmd(this.state.settings[key]); - const path = md.subType === 'file_path_and_args' ? cmd[0] : this.state.settings[key]; - - const argComp = md.subType !== 'file_path_and_args' ? null : ( -
-
{_('Arguments:')}
- { - onArgsChange(event); - }} - value={cmd[1]} - spellCheck={false} - /> -
- {descriptionComp} -
-
- ); - - return ( -
-
- -
-
-
-
-
{_('Path:')}
-
- { - onPathChange(event); - }} - value={path} - spellCheck={false} - /> -
-
- {descriptionComp} -
-
-
-
- {argComp} -
- ); - } else { - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const onTextChange = (event: any) => { - updateSettingValue(key, event.target.value); - }; - return ( -
-
- -
- { - md.subType === SettingItemSubType.FontFamily || md.subType === SettingItemSubType.MonospaceFontFamily ? - updateSettingValue(key, fontFamily)} - subtype={md.subType} - /> : - { - onTextChange(event); - }} - spellCheck={false} - /> - } -
- {descriptionComp} -
-
- ); - } - } else if (md.type === Setting.TYPE_INT) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const onNumChange = (event: any) => { - updateSettingValue(key, event.target.value); - }; - - const label = [md.label()]; - if (md.unitLabel) label.push(`(${md.unitLabel(md.value)})`); - - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - const inputStyle: any = { ...textInputBaseStyle }; - - return ( -
-
- -
- { - onNumChange(event); - }} - min={md.minimum} - max={md.maximum} - step={md.step} - spellCheck={false} - /> - {descriptionComp} -
- ); - } else if (md.type === Setting.TYPE_BUTTON) { - const labelComp = md.hideLabel ? null : ( -
- -
- ); - - return ( -
- {labelComp} -
- ); - } else { - console.warn(`Type not implemented: ${key}`); + if (md.needRestart) { + this.setState({ needRestart: true }); } + shared.updateSettingValue(this, key, value); + }; - return output; + public settingToComponent(key: T, value: SettingValueType) { + return ( + + ); } private restartMessage() { @@ -768,7 +371,7 @@ class ConfigScreenComponent extends React.Component { const settings = this.state.settings; - const containerStyle = { + const containerStyle: React.CSSProperties = { overflow: 'auto', padding: theme.configScreenPadding, paddingTop: 0, @@ -800,6 +403,35 @@ class ConfigScreenComponent extends React.Component { const rightStyle = { ...style, flex: 1 }; delete style.width; + const tabComponents: React.ReactNode[] = []; + for (const section of sections) { + const sectionId = `setting-section-${section.name}`; + let content = null; + const visible = section.name === this.state.selectedSectionName; + if (visible) { + content = ( + <> + {screenComp} +
{settingComps}
+ + ); + } + + tabComponents.push( + , + ); + } + return (
{ sections={sections} />
- {screenComp} {needRestartComp} -
{settingComps}
+ {tabComponents} void; + sections: MetadataBySection; } export const StyledRoot = styled.div` @@ -73,24 +77,63 @@ export const StyledListItemIcon = styled.i` `; export default function Sidebar(props: Props) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied; - const buttons: any[] = []; + const buttonRefs = useRef([]); - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied; - function renderButton(section: any) { + // Making a tabbed region accessible involves supporting keyboard interaction. + // See https://www.w3.org/WAI/ARIA/apg/patterns/tabs/ for details + const onKeyDown: React.KeyboardEventHandler = useCallback((event) => { + const selectedIndex = props.sections.findIndex(section => section.name === props.selection); + let newIndex = selectedIndex; + + if (event.code === 'ArrowUp') { + newIndex --; + } else if (event.code === 'ArrowDown') { + newIndex ++; + } else if (event.code === 'Home') { + newIndex = 0; + } else if (event.code === 'End') { + newIndex = props.sections.length - 1; + } + + if (newIndex < 0) newIndex += props.sections.length; + newIndex %= props.sections.length; + + if (newIndex !== selectedIndex) { + event.preventDefault(); + props.onSelectionChange({ section: props.sections[newIndex] }); + + const targetButton = buttonRefs.current[newIndex]; + if (targetButton) { + focus('Sidebar', targetButton); + } + } + }, [props.sections, props.selection, props.onSelectionChange]); + + const buttons: React.ReactNode[] = []; + + function renderButton(section: SettingMetadataSection, index: number) { const selected = props.selection === section.name; return ( { buttonRefs.current[index] = item; }} + + id={`setting-tab-${section.name}`} + aria-controls={`setting-section-${section.name}`} aria-selected={selected} + tabIndex={selected ? 0 : -1} + isSubSection={Setting.isSubSection(section.name)} selected={selected} onClick={() => { props.onSelectionChange({ section: section }); }} + onKeyDown={onKeyDown} > {Setting.sectionNameToLabel(section.name)} @@ -109,13 +152,15 @@ export default function Sidebar(props: Props) { let pluginDividerAdded = false; + let index = 0; for (const section of props.sections) { if (section.source === SettingSectionSource.Plugin && !pluginDividerAdded) { buttons.push(renderDivider('divider-plugins')); pluginDividerAdded = true; } - buttons.push(renderButton(section)); + buttons.push(renderButton(section, index)); + index ++; } return ( diff --git a/packages/app-desktop/gui/ConfigScreen/FontSearch.tsx b/packages/app-desktop/gui/ConfigScreen/controls/FontSearch.tsx similarity index 99% rename from packages/app-desktop/gui/ConfigScreen/FontSearch.tsx rename to packages/app-desktop/gui/ConfigScreen/controls/FontSearch.tsx index d80a0d192..f7505300c 100644 --- a/packages/app-desktop/gui/ConfigScreen/FontSearch.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/FontSearch.tsx @@ -9,6 +9,7 @@ interface Props { style: CSSProperties; value: string; availableFonts: string[]; + inputId: string; onChange: (font: string)=> void; subtype: string; } @@ -108,6 +109,7 @@ const FontSearch = (props: Props) => { onFocus={handleFocus} onBlur={handleBlur} spellCheck={false} + id={props.inputId} ref={fontInputRef} />
= { + 'plugins.states': control_PluginsStates, +}; + +export interface UpdateSettingValueEvent { + key: string; + value: unknown; +} + +interface Props { + themeId: number; + settingKey: string; + value: unknown; + fonts: string[]; + onUpdateSettingValue: (event: UpdateSettingValueEvent)=> void; + onSettingButtonClick: (key: string)=> void; +} + +const SettingComponent: React.FC = props => { + const theme = themeStyle(props.themeId); + + const output: React.ReactNode = null; + + const updateSettingValue = useCallback((key: string, value: unknown) => { + props.onUpdateSettingValue({ key, value }); + }, [props.onUpdateSettingValue]); + + const rowStyle = { + marginBottom: theme.mainPadding * 1.5, + }; + + const controlStyle = { + display: 'inline-block', + color: theme.color, + fontFamily: theme.fontFamily, + backgroundColor: theme.backgroundColor, + }; + + const textInputBaseStyle: React.CSSProperties = { + ...controlStyle, + fontFamily: theme.fontFamily, + border: '1px solid', + padding: '4px 6px', + boxSizing: 'border-box', + borderColor: theme.borderColor4, + borderRadius: 3, + paddingLeft: 6, + paddingRight: 6, + paddingTop: 4, + paddingBottom: 4, + }; + + const key = props.settingKey; + const md = Setting.settingMetadata(key); + + const descriptionText = Setting.keyDescription(key, AppType.Desktop); + const inputId = useId(); + const descriptionId = useId(); + const descriptionComp = ; + + if (key in settingKeyToControl) { + const CustomSettingComponent = settingKeyToControl[key]; + const label = md.label ? : null; + return ( +
+ {label} + + { + updateSettingValue(key, event.value); + }} + /> +
+ ); + } else if (md.isEnum) { + const value = props.value as string; + + const items = []; + const settingOptions = md.options(); + const array = Setting.enumOptionsToValueLabels(settingOptions, md.optionsOrder ? md.optionsOrder() : [], { + valueKey: 'key', + labelKey: 'label', + }); + + for (let i = 0; i < array.length; i++) { + const e = array[i]; + items.push( + , + ); + } + + const selectStyle = { ...controlStyle, paddingLeft: 6, + paddingRight: 6, + paddingTop: 4, + paddingBottom: 4, + borderColor: theme.borderColor4, + borderRadius: 3 }; + + return ( +
+ + + {descriptionComp} +
+ ); + } else if (md.type === Setting.TYPE_BOOL) { + const value = props.value as boolean; + + const checkboxSize = theme.fontSize * 1.1666666666666; + + return ( +
+
+ updateSettingValue(key, event.target.checked)} + style={{ marginLeft: 0, width: checkboxSize, height: checkboxSize }} + + // Prefer aria-details to aria-describedby for checkbox inputs -- + // on MacOS, VoiceOver reads "checked"/"unchecked" only after reading the + // potentially-lengthy description. For other input types, the input value + // is read first. + aria-details={descriptionId} + /> + +
+ {descriptionComp} +
+ ); + } else if (md.type === Setting.TYPE_STRING) { + const value = props.value as string; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied + const inputStyle: any = { ...textInputBaseStyle, width: '50%', + minWidth: '20em' }; + const inputType = md.secure === true ? 'password' : 'text'; + + if (md.subType === 'file_path_and_args' || md.subType === 'file_path' || md.subType === 'directory_path') { + inputStyle.marginBottom = theme.mainPadding / 2; + + const splitCmd = (cmdString: string) => { + // Normally not necessary but certain plugins found a way to + // set the set the value to "undefined", leading to a crash. + // This is now fixed at the model level but to be sure we + // check here too, to handle any already existing data. + // https://github.com/laurent22/joplin/issues/7621 + if (!cmdString) cmdString = ''; + const path = pathUtils.extractExecutablePath(cmdString); + const args = cmdString.substr(path.length + 1); + return [pathUtils.unquotePath(path), args]; + }; + + const joinCmd = (cmdArray: string[]) => { + if (!cmdArray[0] && !cmdArray[1]) return ''; + let cmdString = pathUtils.quotePath(cmdArray[0]); + if (!cmdString) cmdString = '""'; + if (cmdArray[1]) cmdString += ` ${cmdArray[1]}`; + return cmdString; + }; + + const onPathChange: React.ChangeEventHandler = event => { + if (md.subType === 'file_path_and_args') { + const cmd = splitCmd(value); + cmd[0] = event.target.value; + updateSettingValue(key, joinCmd(cmd)); + } else { + updateSettingValue(key, event.target.value); + } + }; + + const onArgsChange: React.ChangeEventHandler = event => { + const cmd = splitCmd(value); + cmd[1] = event.target.value; + updateSettingValue(key, joinCmd(cmd)); + }; + + const browseButtonClick = async () => { + if (md.subType === 'directory_path') { + const paths = await bridge().showOpenDialog({ + properties: ['openDirectory'], + }); + if (!paths || !paths.length) return; + updateSettingValue(key, paths[0]); + } else { + const paths = await bridge().showOpenDialog(); + if (!paths || !paths.length) return; + + if (md.subType === 'file_path') { + updateSettingValue(key, paths[0]); + } else { + const cmd = splitCmd(value); + cmd[0] = paths[0]; + updateSettingValue(key, joinCmd(cmd)); + } + } + }; + + const cmd = splitCmd(value); + const path = md.subType === 'file_path_and_args' ? cmd[0] : value; + + const argInputId = `setting_path_arg_${key}`; + const argComp = md.subType !== 'file_path_and_args' ? null : ( +
+ + +
+ {descriptionComp} +
+
+ ); + + const pathDescriptionId = `setting_path_label_${key}`; + return ( +
+ +
+
+
+
{_('Path:')}
+
+ +
+
+ {descriptionComp} +
+
+
+
+ {argComp} +
+ ); + } else { + const onTextChange: React.ChangeEventHandler = event => { + updateSettingValue(key, event.target.value); + }; + return ( +
+ + { + md.subType === SettingItemSubType.FontFamily || md.subType === SettingItemSubType.MonospaceFontFamily ? + updateSettingValue(key, fontFamily)} + subtype={md.subType} + inputId={inputId} + /> : + + } +
+ {descriptionComp} +
+
+ ); + } + } else if (md.type === Setting.TYPE_INT) { + const value = props.value as number; + + const onNumChange: React.ChangeEventHandler = (event) => { + updateSettingValue(key, event.target.value); + }; + + const label = [md.label()]; + if (md.unitLabel) label.push(`(${md.unitLabel(md.value)})`); + + return ( +
+ + + {descriptionComp} +
+ ); + } else if (md.type === Setting.TYPE_BUTTON) { + const labelComp = md.hideLabel ? null : ( + + ); + + return ( +
+ {labelComp} +
+ ); + } else { + console.warn(`Type not implemented: ${key}`); + } + + return output; +}; + +export default SettingComponent; diff --git a/packages/app-desktop/gui/ConfigScreen/controls/SettingDescription.tsx b/packages/app-desktop/gui/ConfigScreen/controls/SettingDescription.tsx new file mode 100644 index 000000000..505d67681 --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/controls/SettingDescription.tsx @@ -0,0 +1,12 @@ +import * as React from 'react'; + +interface Props { + text: string; + id?: string; +} + +const SettingDescription: React.FC = props => { + return props.text ?
{props.text}
: null; +}; + +export default SettingDescription; diff --git a/packages/app-desktop/gui/ConfigScreen/controls/SettingHeader.tsx b/packages/app-desktop/gui/ConfigScreen/controls/SettingHeader.tsx new file mode 100644 index 000000000..e97d0ab5f --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/controls/SettingHeader.tsx @@ -0,0 +1,15 @@ +import * as React from 'react'; + +interface Props { + text: string; +} + +const SettingHeader: React.FC = props => { + return ( +
+ +
+ ); +}; + +export default SettingHeader; diff --git a/packages/app-desktop/gui/ConfigScreen/controls/SettingLabel.tsx b/packages/app-desktop/gui/ConfigScreen/controls/SettingLabel.tsx new file mode 100644 index 000000000..a0afeb08f --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/controls/SettingLabel.tsx @@ -0,0 +1,16 @@ +import * as React from 'react'; + +interface Props { + htmlFor: string|null; + text: string; +} + +const SettingLabel: React.FC = props => { + return ( +
+ +
+ ); +}; + +export default SettingLabel; diff --git a/packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.tsx b/packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.tsx index 26a19eb05..d8017fc15 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.tsx @@ -6,6 +6,7 @@ import { _ } from '@joplin/lib/locale'; interface Props { onClick: ()=> void; advancedSettingsVisible: boolean; + 'aria-controls': string; } const ToggleAdvancedSettingsButton: React.FunctionComponent = props => { @@ -16,6 +17,10 @@ const ToggleAdvancedSettingsButton: React.FunctionComponent = props => { level={ButtonLevel.Secondary} onClick={props.onClick} iconName={iconName} + + aria-controls={props['aria-controls']} + aria-expanded={props.advancedSettingsVisible} + title={_('Show Advanced Settings')} />
diff --git a/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.tsx b/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.tsx index 356cf8714..cf728e6ca 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useCallback, useMemo } from 'react'; +import { useCallback, useId, useMemo } from 'react'; import { _ } from '@joplin/lib/locale'; import styled from 'styled-components'; import ToggleButton from '../../../lib/ToggleButton/ToggleButton'; @@ -173,6 +173,7 @@ export default function(props: Props) { themeId={props.themeId} value={item.enabled} onToggle={() => props.onToggle({ item })} + aria-label={_('Enabled')} />; } @@ -256,10 +257,17 @@ export default function(props: Props) { return ; } + const nameLabelId = useId(); + return ( - + - {item.manifest.name} {item.deleted ? _('(%s)', 'Deleted') : ''}v{item.manifest.version} + + + {item.manifest.name} {item.deleted ? _('(%s)', 'Deleted') : ''} + + v{item.manifest.version} + {renderToggleButton()} {renderRecommendedBadge()} diff --git a/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx b/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx index 119efe79a..6e5165976 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx @@ -17,6 +17,8 @@ import useOnDeleteHandler from '@joplin/lib/components/shared/config/plugins/use import Logger from '@joplin/utils/Logger'; import StyledMessage from '../../../style/StyledMessage'; import StyledLink from '../../../style/StyledLink'; +import SettingHeader from '../SettingHeader'; +import SettingDescription from '../SettingDescription'; const { space } = require('styled-system'); const logger = Logger.create('PluginState'); @@ -51,12 +53,6 @@ interface Props { themeId: number; // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied onChange: Function; - // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied - renderLabel: Function; - // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied - renderDescription: Function; - // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied - renderHeader: Function; } let repoApi_: RepositoryApi = null; @@ -281,7 +277,7 @@ export default function(props: Props) { if (!pluginItems.length || allDeleted) { return ( - {props.renderDescription(props.themeId, _('You do not have any installed plugin.'))} + ); } else { @@ -311,7 +307,6 @@ export default function(props: Props) { pluginSettings={pluginSettings} onSearchQueryChange={onSearchQueryChange} onPluginSettingsChange={onSearchPluginSettingsChange} - renderDescription={props.renderDescription} repoApi={repoApi} />
@@ -333,7 +328,7 @@ export default function(props: Props) {
- {props.renderHeader(props.themeId, _('Manage your plugins'))} +
{renderUserPlugins(pluginItems)} diff --git a/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx b/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx index 0190dffd6..fb9534d26 100644 --- a/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx +++ b/packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.tsx @@ -10,6 +10,7 @@ import PluginService, { PluginSettings } from '@joplin/lib/services/plugins/Plug import { _ } from '@joplin/lib/locale'; import useOnInstallHandler from '@joplin/lib/components/shared/config/plugins/useOnInstallHandler'; import { themeStyle } from '@joplin/lib/theme'; +import SettingDescription from '../SettingDescription'; const Root = styled.div` `; @@ -26,8 +27,6 @@ interface Props { pluginSettings: PluginSettings; // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied onPluginSettingsChange(event: any): void; - // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied - renderDescription: Function; maxWidth: number; repoApi(): RepositoryApi; disabled: boolean; @@ -81,7 +80,7 @@ export default function(props: Props) { function renderResults(query: string, manifests: PluginManifest[]) { if (query && !manifests.length) { if (searchResultCount === null) return ''; // Search in progress - return props.renderDescription(props.themeId, _('No results')); + return ; } else { const output = []; diff --git a/packages/app-desktop/gui/ConfigScreen/style.scss b/packages/app-desktop/gui/ConfigScreen/style.scss index a6b5f367a..82f83487a 100644 --- a/packages/app-desktop/gui/ConfigScreen/style.scss +++ b/packages/app-desktop/gui/ConfigScreen/style.scss @@ -1,3 +1,5 @@ +@use "./styles/index.scss"; + .config-screen-content-wrapper { padding: 24px; overflow: auto; diff --git a/packages/app-desktop/gui/ConfigScreen/styles/index.scss b/packages/app-desktop/gui/ConfigScreen/styles/index.scss new file mode 100644 index 000000000..6eba6a8a6 --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/styles/index.scss @@ -0,0 +1,5 @@ + +@use "./setting-description.scss"; +@use "./setting-label.scss"; +@use "./setting-header.scss"; +@use "./setting-tab-panel.scss"; diff --git a/packages/app-desktop/gui/ConfigScreen/styles/setting-description.scss b/packages/app-desktop/gui/ConfigScreen/styles/setting-description.scss new file mode 100644 index 000000000..457e17ead --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/styles/setting-description.scss @@ -0,0 +1,9 @@ + +.setting-description { + color: var(--joplin-color-faded); + font-size: var(--joplin-font-size); + line-height: var(--joplin-line-height); + font-style: italic; + max-width: 70em; + margin-top: 5px; +} diff --git a/packages/app-desktop/gui/ConfigScreen/styles/setting-header.scss b/packages/app-desktop/gui/ConfigScreen/styles/setting-header.scss new file mode 100644 index 000000000..d26d322b8 --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/styles/setting-header.scss @@ -0,0 +1,9 @@ + +.setting-header { + display: block; + color: var(--joplin-color); + font-size: calc(var(--joplin-font-size) * 1.25); + font-weight: 500; + margin-bottom: var(--joplin-main-padding); + line-height: var(--joplin-line-height); +} \ No newline at end of file diff --git a/packages/app-desktop/gui/ConfigScreen/styles/setting-label.scss b/packages/app-desktop/gui/ConfigScreen/styles/setting-label.scss new file mode 100644 index 000000000..fd1893bee --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/styles/setting-label.scss @@ -0,0 +1,20 @@ + +.setting-label { + display: block; + color: var(--joplin-color); + font-size: calc(var(--joplin-font-size) * 1.083333); + font-weight: 500; + margin-bottom: calc(var(--joplin-main-padding) / 2); + line-height: var(--joplin-line-height); + + &.-sub-label { + opacity: 0.7; + } + + &.-for-checkbox { + margin-left: 5px; + margin-bottom: 0; + display: inline; + background-color: transparent; + } +} \ No newline at end of file diff --git a/packages/app-desktop/gui/ConfigScreen/styles/setting-tab-panel.scss b/packages/app-desktop/gui/ConfigScreen/styles/setting-tab-panel.scss new file mode 100644 index 000000000..7febf2e0d --- /dev/null +++ b/packages/app-desktop/gui/ConfigScreen/styles/setting-tab-panel.scss @@ -0,0 +1,18 @@ + +.setting-tab-panel { + display: flex; + flex-grow: 1; + flex-shrink: 1; + min-height: 0; + + &.-hidden { + display: none; + } + + &:focus-visible { + // Use a border rather than an outline -- an outline would be shown outside of the screen + // and thus invisible. + border: 1px solid var(--joplin-focus-outline-color); + outline: none; + } +} \ No newline at end of file diff --git a/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx b/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx index 5f3f9c422..c21d41a18 100644 --- a/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx +++ b/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx @@ -10,7 +10,7 @@ import { MasterKeyEntity } from '@joplin/lib/services/e2ee/types'; import { getEncryptionEnabled, masterKeyEnabled, SyncInfo } from '@joplin/lib/services/synchronizer/syncInfoUtils'; import { getDefaultMasterKey, getMasterPasswordStatusMessage, masterPasswordIsValid, toggleAndSetupEncryption } from '@joplin/lib/services/e2ee/utils'; import Button, { ButtonLevel } from '../Button/Button'; -import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useId, useMemo, useState } from 'react'; import { connect } from 'react-redux'; import { AppState } from '../../app.reducer'; import Setting from '@joplin/lib/models/Setting'; @@ -350,7 +350,7 @@ const EncryptionConfigScreen = (props: Props) => { t = `

${t}

`; return ( -
+ <>

{_('Re-encryption')}

@@ -358,7 +358,7 @@ const EncryptionConfigScreen = (props: Props) => { { !props.shouldReencrypt ? null : } -
+ ); }; @@ -368,6 +368,7 @@ const EncryptionConfigScreen = (props: Props) => { setShowAdvanced(!showAdvanced); }, [showAdvanced]); + const advancedSettingsId = useId(); const renderAdvancedSection = () => { const reEncryptSection = renderReencryptData(); @@ -378,8 +379,12 @@ const EncryptionConfigScreen = (props: Props) => {
- { showAdvanced ? reEncryptSection : null } + advancedSettingsVisible={showAdvanced} + aria-controls={advancedSettingsId} + /> +
+ { showAdvanced ? reEncryptSection : null } +
); }; diff --git a/packages/app-desktop/gui/lib/ToggleButton/ToggleButton.tsx b/packages/app-desktop/gui/lib/ToggleButton/ToggleButton.tsx index 80450f330..e8b5f3b44 100644 --- a/packages/app-desktop/gui/lib/ToggleButton/ToggleButton.tsx +++ b/packages/app-desktop/gui/lib/ToggleButton/ToggleButton.tsx @@ -1,5 +1,6 @@ import { themeStyle } from '@joplin/lib/theme'; import * as React from 'react'; +import { useMemo } from 'react'; const ReactToggleButton = require('react-toggle-button'); const Color = require('color'); @@ -8,11 +9,27 @@ interface Props { // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied onToggle: Function; themeId: number; + 'aria-label': string; } export default function(props: Props) { const theme = themeStyle(props.themeId); + const ariaLabel = props['aria-label']; + + const passThroughInputProps = useMemo(() => { + return { + 'aria-label': ariaLabel, + + // Works around a bug in ReactToggleButton -- the hidden checkbox input associated + // with the toggle is always read as "unchecked" by screen readers. + checked: props.value, + // Silences a ReactJS warning: "You provided a `checked` prop to a form field without an `onChange` handler." + // Change events are handled by ReactToggleButton. + onChange: ()=>{}, + }; + }, [ariaLabel, props.value]); + return ( ); } diff --git a/packages/app-desktop/integration-tests/main.spec.ts b/packages/app-desktop/integration-tests/main.spec.ts index 40b1403bd..10a80b62f 100644 --- a/packages/app-desktop/integration-tests/main.spec.ts +++ b/packages/app-desktop/integration-tests/main.spec.ts @@ -1,6 +1,5 @@ import { test, expect } from './util/test'; import MainScreen from './models/MainScreen'; -import SettingsScreen from './models/SettingsScreen'; import { _electron as electron } from '@playwright/test'; import { writeFile } from 'fs-extra'; import { join } from 'path'; @@ -130,36 +129,6 @@ test.describe('main', () => { expect(fullSize[0] / resizedSize[0]).toBeCloseTo(fullSize[1] / resizedSize[1]); }); - test('should be possible to remove sort order buttons in settings', async ({ electronApp, mainWindow }) => { - const mainScreen = new MainScreen(mainWindow); - await mainScreen.waitFor(); - - // Sort order buttons should be visible by default - await expect(mainScreen.noteListContainer.locator('[title^="Toggle sort order"]')).toBeVisible(); - - await mainScreen.openSettings(electronApp); - - // Should be on the settings screen - const settingsScreen = new SettingsScreen(mainWindow); - await settingsScreen.waitFor(); - - // Open the appearance tab - await settingsScreen.appearanceTabButton.click(); - - // Find the sort order visible checkbox - const sortOrderVisibleCheckbox = mainWindow.getByLabel(/^Show sort order/); - - await expect(sortOrderVisibleCheckbox).toBeChecked(); - await sortOrderVisibleCheckbox.click(); - await expect(sortOrderVisibleCheckbox).not.toBeChecked(); - - // Save settings & close - await settingsScreen.okayButton.click(); - await mainScreen.waitFor(); - - await expect(mainScreen.noteListContainer.locator('[title^="Toggle sort order"]')).not.toBeVisible(); - }); - test('clicking on an external link should try to launch a browser', async ({ electronApp, mainWindow }) => { const mainScreen = new MainScreen(mainWindow); await mainScreen.waitFor(); diff --git a/packages/app-desktop/integration-tests/models/MainScreen.ts b/packages/app-desktop/integration-tests/models/MainScreen.ts index a1c721252..0a8a4f5bd 100644 --- a/packages/app-desktop/integration-tests/models/MainScreen.ts +++ b/packages/app-desktop/integration-tests/models/MainScreen.ts @@ -17,7 +17,7 @@ export default class MainScreen { this.newNoteButton = page.locator('.new-note-button'); this.noteListContainer = page.locator('.rli-noteList'); this.sidebar = new Sidebar(page, this); - this.dialog = page.locator('.dialog-root'); + this.dialog = page.locator('.dialog-modal-layer'); this.noteEditor = new NoteEditorScreen(page); this.goToAnything = new GoToAnything(page, this); } diff --git a/packages/app-desktop/integration-tests/models/SettingsScreen.ts b/packages/app-desktop/integration-tests/models/SettingsScreen.ts index dcc0e97b1..7cee88390 100644 --- a/packages/app-desktop/integration-tests/models/SettingsScreen.ts +++ b/packages/app-desktop/integration-tests/models/SettingsScreen.ts @@ -11,7 +11,7 @@ export default class SettingsScreen { } public getTabLocator(tabName: string) { - return this.page.locator('a[role="tab"] > span', { hasText: tabName }); + return this.page.getByRole('tab', { name: tabName }); } public async waitFor() { diff --git a/packages/app-desktop/integration-tests/settings.spec.ts b/packages/app-desktop/integration-tests/settings.spec.ts new file mode 100644 index 000000000..9b6d44e81 --- /dev/null +++ b/packages/app-desktop/integration-tests/settings.spec.ts @@ -0,0 +1,96 @@ +import { test, expect } from './util/test'; +import MainScreen from './models/MainScreen'; +import SettingsScreen from './models/SettingsScreen'; + +test.describe('settings', () => { + test('should be possible to remove sort order buttons in settings', async ({ electronApp, mainWindow }) => { + const mainScreen = new MainScreen(mainWindow); + await mainScreen.waitFor(); + + // Sort order buttons should be visible by default + const sortOrderLocator = mainScreen.noteListContainer.locator('[title^="Toggle sort order"]'); + await expect(sortOrderLocator).toBeVisible(); + + await mainScreen.openSettings(electronApp); + + // Should be on the settings screen + const settingsScreen = new SettingsScreen(mainWindow); + await settingsScreen.waitFor(); + + // Open the appearance tab + await settingsScreen.appearanceTabButton.click(); + + // Find the sort order visible checkbox + const sortOrderVisibleCheckbox = mainWindow.getByLabel(/^Show sort order/); + + await expect(sortOrderVisibleCheckbox).toBeChecked(); + await sortOrderVisibleCheckbox.click(); + await expect(sortOrderVisibleCheckbox).not.toBeChecked(); + + // Save settings & close + await settingsScreen.okayButton.click(); + await mainScreen.waitFor(); + + await expect(sortOrderLocator).not.toBeVisible(); + }); + + test('clicking the sync wizard button in settings should open a dialog', async ({ electronApp, mainWindow }) => { + const mainScreen = new MainScreen(mainWindow); + await mainScreen.waitFor(); + await mainScreen.openSettings(electronApp); + + const settingsScreen = new SettingsScreen(mainWindow); + const generalTab = settingsScreen.getTabLocator('Synchronisation'); + await generalTab.click(); + + await expect(mainScreen.dialog).not.toBeVisible(); + + const syncWizardButton = mainWindow.getByRole('button', { name: 'Open Sync Wizard' }); + await syncWizardButton.click(); + + await expect(mainScreen.dialog).toBeVisible(); + }); + + test('should be possible to navigate settings screen tabs with the arrow keys', async ({ electronApp, mainWindow }) => { + const mainScreen = new MainScreen(mainWindow); + await mainScreen.waitFor(); + await mainScreen.openSettings(electronApp); + + const settingsScreen = new SettingsScreen(mainWindow); + await settingsScreen.waitFor(); + + const generalTab = settingsScreen.getTabLocator('General'); + await generalTab.click(); + + const focusedItem = mainWindow.locator(':focus'); + + // Up/Down arrows should move to the next and previous items + await expect(focusedItem).toHaveText('General'); + await mainWindow.keyboard.press('ArrowDown'); + await expect(focusedItem).toHaveText('Application'); + await mainWindow.keyboard.press('ArrowUp'); + await expect(focusedItem).toHaveText('General'); + + // Pressing Up when the first item is focused should focus the last item + await mainWindow.keyboard.press('ArrowUp'); + await expect(focusedItem).toHaveText('Backup'); + + await mainWindow.keyboard.press('ArrowDown'); + await mainWindow.keyboard.press('ArrowDown'); + + await expect(focusedItem).toHaveText('Application'); + + // Pressing Tab should focus the tab container + await mainWindow.keyboard.press('Tab'); + await expect(focusedItem).toHaveAttribute('role', 'tabpanel'); + + // The correct tab should be visible + await expect(mainWindow.getByLabel('Show tray icon')).toBeVisible(); + + // Shift+Tab should focus the sidebar again + await mainWindow.keyboard.press('Shift+Tab'); + await expect(focusedItem).toHaveAttribute('role', 'tab'); + await expect(focusedItem).toHaveText('Application'); + }); +}); + diff --git a/packages/app-desktop/main.scss b/packages/app-desktop/main.scss index 3fa523a43..d99d8188a 100644 --- a/packages/app-desktop/main.scss +++ b/packages/app-desktop/main.scss @@ -143,7 +143,7 @@ a { *:focus-visible { - outline: 1px solid var(--joplin-color-warn); + outline: 1px solid var(--joplin-focus-outline-color); } // The browser-default focus-visible indicator was originally removed for aesthetic diff --git a/packages/lib/models/Setting.ts b/packages/lib/models/Setting.ts index 472991cba..8fb362383 100644 --- a/packages/lib/models/Setting.ts +++ b/packages/lib/models/Setting.ts @@ -17,7 +17,7 @@ const logger = Logger.create('models/Setting'); export * from './settings/types'; -type SettingValueType = ( +export type SettingValueType = ( T extends BuiltInMetadataKeys ? BuiltInMetadataValues[T] // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partial refactor of old code before rule was applied diff --git a/packages/lib/theme.ts b/packages/lib/theme.ts index f3b8986e5..590054cd9 100644 --- a/packages/lib/theme.ts +++ b/packages/lib/theme.ts @@ -110,6 +110,7 @@ export function extraStyles(theme: Theme) { const bgColor4 = theme.backgroundColor4; const borderColor4: string = Color(theme.color).alpha(0.3); const iconColor = Color(theme.color).alpha(0.8); + const focusOutlineColor = theme.colorWarn; const backgroundColor5 = theme.backgroundColor5 ?? theme.color4; const backgroundColorHover5 = Color(backgroundColor5).darken(0.2).hex(); @@ -230,6 +231,7 @@ export function extraStyles(theme: Theme) { backgroundColor5, backgroundColorHover5, backgroundColorActive5, + focusOutlineColor, icon: { ...globalStyle.icon,