diff --git a/client/src/features/reader/cbz/CbzReaderView.vue b/client/src/features/reader/cbz/CbzReaderView.vue index 6989e8389..e82bdfc61 100644 --- a/client/src/features/reader/cbz/CbzReaderView.vue +++ b/client/src/features/reader/cbz/CbzReaderView.vue @@ -16,7 +16,7 @@ import type { CbxReaderSettings } from '@bookorbit/types' import { DEFAULT_WIDE_PAGE_RATIO_THRESHOLD, createCbzSpreadLayout } from './lib/spread-layout' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' -import { Sheet, SheetContent } from '@/components/ui/sheet' +import ReaderSettingsSheet from '@/features/reader/shared/components/ReaderSettingsSheet.vue' import CbzSettingsPanel from './components/CbzSettingsPanel.vue' const TWO_PAGE_BREAKPOINT = 900 @@ -99,8 +99,9 @@ function onSettingsOpenChange(open: boolean) { showSettings.value = open } -function openSettings() { - showSettings.value = true +/** Mirrors PopoverTrigger on the wide path, so the icon means the same thing in both containers. */ +function toggleSettings() { + showSettings.value = !showSettings.value } function applySettings(s: CbxReaderSettings) { @@ -768,29 +769,19 @@ onUnmounted(() => { :class="showSettings ? '!bg-muted !text-foreground' : ''" :title="t('reader.settings.title')" :aria-label="t('reader.settings.ariaLabel')" - @click="openSettings" + @click="toggleSettings" > - - -
-
-
- - - + + + diff --git a/client/src/features/reader/epub/components/ReaderHeader.vue b/client/src/features/reader/epub/components/ReaderHeader.vue index 10f85f07a..49cf8dfff 100644 --- a/client/src/features/reader/epub/components/ReaderHeader.vue +++ b/client/src/features/reader/epub/components/ReaderHeader.vue @@ -16,8 +16,8 @@ import { } from '@lucide/vue' import { useMediaQuery } from '@vueuse/core' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' -import { Sheet, SheetContent } from '@/components/ui/sheet' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' +import ReaderSettingsSheet from '@/features/reader/shared/components/ReaderSettingsSheet.vue' import { useFullscreen } from '../../shared/composables/useFullscreen' const { t } = useI18n() @@ -52,8 +52,9 @@ function onSettingsOpenChange(open: boolean) { emit('update:settingsOpen', open) } -function openSettings() { - emit('update:settingsOpen', true) +/** Mirrors PopoverTrigger on the wide path, so the icon means the same thing in both containers. */ +function toggleSettings() { + emit('update:settingsOpen', !props.settingsOpen) } function getFooterModeIcon(mode: 0 | 1 | 2) { @@ -175,23 +176,13 @@ function getFooterModeTooltip(mode: 0 | 1 | 2): string { :class="props.settingsOpen ? '!bg-muted !text-foreground' : ''" :title="t('reader.settings.title')" :aria-label="t('reader.settings.ariaLabel')" - @click="openSettings" + @click="toggleSettings" > - - -
-
-
- - - + + + diff --git a/client/src/features/reader/epub/components/__tests__/ReaderHeader.spec.ts b/client/src/features/reader/epub/components/__tests__/ReaderHeader.spec.ts index 7d6c24987..459898055 100644 --- a/client/src/features/reader/epub/components/__tests__/ReaderHeader.spec.ts +++ b/client/src/features/reader/epub/components/__tests__/ReaderHeader.spec.ts @@ -19,6 +19,7 @@ const PopoverStub = defineComponent({ const SheetStub = defineComponent({ name: 'SettingsSheet', + props: { open: { type: Boolean, default: false } }, emits: ['update:open'], template: '
', }) @@ -31,8 +32,7 @@ const global = { Popover: PopoverStub, PopoverTrigger: { template: '
' }, PopoverContent: { template: '
' }, - Sheet: SheetStub, - SheetContent: { template: '
' }, + ReaderSettingsSheet: SheetStub, }, } @@ -45,6 +45,7 @@ function mountHeader(props: Record = {}) { footerMode: 0, ...props, }, + slots: { settingsPanel: '

Panel body

' }, global, }) } @@ -98,5 +99,38 @@ describe('ReaderHeader', () => { viewport.isCompact = false }) + + it('renders the settings panel inside the compact sheet', () => { + viewport.isCompact = true + const wrapper = mountHeader({ settingsOpen: true }) + + expect(wrapper.findComponent(SheetStub).find('[data-testid="settings-panel"]').exists()).toBe(true) + + viewport.isCompact = false + }) + + it('forwards the open state to the compact sheet', async () => { + viewport.isCompact = true + const wrapper = mountHeader() + expect(wrapper.findComponent(SheetStub).props('open')).toBe(false) + + await wrapper.setProps({ settingsOpen: true }) + expect(wrapper.findComponent(SheetStub).props('open')).toBe(true) + + viewport.isCompact = false + }) + + it('toggles rather than only opening when the compact trigger is pressed', async () => { + // The compact trigger used to hard-set `true`, so it could never take the panel back down. + // The wide path gets this for free from PopoverTrigger; the compact path has to do it itself. + viewport.isCompact = true + const wrapper = mountHeader({ settingsOpen: true }) + + await wrapper.get('button[aria-label="Reader settings"]').trigger('click') + + expect(wrapper.emitted('update:settingsOpen')?.[0]).toEqual([false]) + + viewport.isCompact = false + }) }) }) diff --git a/client/src/features/reader/shared/components/ReaderSettingsSheet.vue b/client/src/features/reader/shared/components/ReaderSettingsSheet.vue new file mode 100644 index 000000000..852ffbbb8 --- /dev/null +++ b/client/src/features/reader/shared/components/ReaderSettingsSheet.vue @@ -0,0 +1,51 @@ + + + diff --git a/client/src/features/reader/shared/components/__tests__/ReaderSettingsSheet.spec.ts b/client/src/features/reader/shared/components/__tests__/ReaderSettingsSheet.spec.ts new file mode 100644 index 000000000..12d88513b --- /dev/null +++ b/client/src/features/reader/shared/components/__tests__/ReaderSettingsSheet.spec.ts @@ -0,0 +1,115 @@ +import { mount } from '@vue/test-utils' +import { defineComponent } from 'vue' +import { describe, expect, it } from 'vitest' +import ReaderSettingsSheet from '../ReaderSettingsSheet.vue' + +const SheetStub = defineComponent({ + name: 'SheetStub', + props: { open: { type: Boolean, default: false } }, + emits: ['update:open'], + template: '
', +}) + +const SheetContentStub = defineComponent({ + name: 'SheetContentStub', + template: '
', +}) + +function mountSheet(props: Record = {}) { + return mount(ReaderSettingsSheet, { + props: { open: true, ...props }, + slots: { default: '

Panel body

' }, + global: { stubs: { Sheet: SheetStub, SheetContent: SheetContentStub } }, + }) +} + +describe('ReaderSettingsSheet', () => { + it('renders the settings panel passed into the default slot', () => { + const wrapper = mountSheet() + + expect(wrapper.get('[data-testid="panel"]').text()).toBe('Panel body') + }) + + it('forwards the open state to the sheet', async () => { + const wrapper = mountSheet({ open: false }) + expect(wrapper.findComponent(SheetStub).props('open')).toBe(false) + + await wrapper.setProps({ open: true }) + expect(wrapper.findComponent(SheetStub).props('open')).toBe(true) + }) + + describe('closing', () => { + // The reported bug: `hide-close` dropped the built-in X and nothing replaced it, so on a phone + // there was no reachable way out of the panel at all. Every assertion here guards one of the + // three exits the reporter found missing. + it('offers a close button with an accessible label', () => { + const wrapper = mountSheet() + const close = wrapper.get('button[aria-label="Close settings"]') + + expect(close.element.tagName).toBe('BUTTON') + expect(close.attributes('type')).toBe('button') + }) + + it('emits update:open false when the close button is activated', async () => { + const wrapper = mountSheet() + + await wrapper.get('button[aria-label="Close settings"]').trigger('click') + + expect(wrapper.emitted('update:open')).toEqual([[false]]) + }) + + it('forwards the sheet dismissing itself, which covers overlay taps and Escape', () => { + const wrapper = mountSheet() + + wrapper.findComponent(SheetStub).vm.$emit('update:open', false) + + expect(wrapper.emitted('update:open')).toEqual([[false]]) + }) + + it('emits nothing until something is actually activated', () => { + const wrapper = mountSheet() + + expect(wrapper.emitted('update:open')).toBeUndefined() + }) + + it('keeps the close button as the only interactive control in the sheet chrome', () => { + const wrapper = mountSheet() + const buttons = wrapper.findAll('button') + + expect(buttons).toHaveLength(1) + expect(buttons[0].attributes('aria-label')).toBe('Close settings') + }) + + it('leaves the grab handle decorative rather than passing it off as a control', () => { + // The reporter tried to drag this and it did nothing. There is no swipe-to-dismiss, so the + // handle must stay hidden from assistive tech instead of advertising an exit that is not there. + const wrapper = mountSheet() + const handle = wrapper.get('[data-testid="sheet-content"] .rounded-full') + + expect(handle.element.tagName).toBe('DIV') + expect(handle.attributes('aria-hidden')).toBe('true') + }) + }) + + describe('sheet geometry', () => { + it('caps its height against the dynamic viewport, not the large viewport', () => { + // `vh` resolves against the toolbar-retracted viewport. On a phone browser with its chrome + // showing, `max-h-[85vh]` covered essentially the whole visible area and left no overlay to + // tap, which is why the panel could only be dismissed in fullscreen. Asserting the class is + // crude, but jsdom has no layout, and this is the exact token that regressed. + const content = mountSheet().get('[data-testid="sheet-content"]') + + expect(content.classes()).toContain('max-h-[85dvh]') + expect(content.classes()).not.toContain('max-h-[85vh]') + }) + + it('suppresses the built-in corner close so it cannot collide with the panel header', () => { + // Paired deliberately with the close-button tests above: `hide-close` is only defensible + // because this component supplies its own close. + const content = mountSheet().get('[data-testid="sheet-content"]') + + expect(Object.keys(content.attributes())).toContain('hide-close') + expect(content.attributes('side')).toBe('bottom') + }) + }) +}) diff --git a/client/src/locales/en.json b/client/src/locales/en.json index 0b9fa580f..f575bea46 100644 --- a/client/src/locales/en.json +++ b/client/src/locales/en.json @@ -6269,6 +6269,7 @@ "settings": { "title": "Settings", "ariaLabel": "Reader settings", + "close": "Close settings", "reset": "Reset to defaults", "modeLabel": "Color mode", "mode": {