From 30a863c79d5a9d6ec52afb554c7e8b6abe0d45d3 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 28 Oct 2026 07:13:34 +0100 Subject: [PATCH] fix: put the template, placeholder and composer dialogs on the shared dialog: close X at the end, Escape, outside click, and a discard prompt for typed changes --- .../email/__tests__/composer-title.test.tsx | 8 + components/email/email-composer.tsx | 313 +++++++--------- .../__tests__/template-dialogs.test.tsx | 132 +++++++ .../templates/placeholder-fill-modal.tsx | 89 +++-- components/templates/template-form.tsx | 129 +++---- .../templates/template-manager-modal.tsx | 343 ++++++++---------- locales/ar/common.json | 3 + locales/ca/common.json | 3 + locales/cs/common.json | 3 + locales/da/common.json | 3 + locales/de/common.json | 3 + locales/en/common.json | 11 +- locales/es/common.json | 3 + locales/et/common.json | 3 + locales/fa/common.json | 3 + locales/fr/common.json | 3 + locales/he/common.json | 3 + locales/hu/common.json | 3 + locales/it/common.json | 3 + locales/ja/common.json | 3 + locales/ko/common.json | 3 + locales/lv/common.json | 3 + locales/mn/common.json | 3 + locales/nb/common.json | 3 + locales/nl/common.json | 3 + locales/pl/common.json | 3 + locales/pt/common.json | 3 + locales/ro/common.json | 3 + locales/ru/common.json | 3 + locales/sk/common.json | 3 + locales/th/common.json | 3 + locales/tr/common.json | 3 + locales/uk/common.json | 3 + locales/zh-TW/common.json | 3 + locales/zh/common.json | 3 + 35 files changed, 617 insertions(+), 492 deletions(-) create mode 100644 components/templates/__tests__/template-dialogs.test.tsx diff --git a/components/email/__tests__/composer-title.test.tsx b/components/email/__tests__/composer-title.test.tsx index 8f5e80ee9..cebc908c1 100644 --- a/components/email/__tests__/composer-title.test.tsx +++ b/components/email/__tests__/composer-title.test.tsx @@ -226,4 +226,12 @@ describe('composer title', () => { render(); expect(screen.getByRole('button', { name: 'close' })).toBeInTheDocument(); }); + + // R4: Close at the top end. On a phone it moves to the start (Send is at the end there). + it('puts Close at the end of the header, and at the start only on a phone', () => { + render(); + const close = screen.getByRole('button', { name: 'close' }); + expect(close.parentElement!.lastElementChild).toBe(close); + expect(close.className).toContain('max-md:order-first'); + }); }); diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index 85119f443..f027b8139 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -2,10 +2,12 @@ import React, { useState, useEffect, useRef, useCallback, useMemo } from "react"; import { useMvComposeWindow } from "@/components/email/mv-compose-context"; -import { useFocusTrap } from "@/hooks/use-focus-trap"; import { useTranslations } from "next-intl"; import { Button, TOUCH_HIT_AREA } from "@/components/ui/button"; import { ConfirmDialog } from "@/components/ui/confirm-dialog"; +import { Dialog } from "@/components/ui/dialog"; +import { IconButton } from "@/components/ui/icon-button"; +import { useDiscardChanges } from "@/components/settings/use-discard-changes"; import { useConfirmDialog } from "@/hooks/use-confirm-dialog"; import { Input } from "@/components/ui/input"; import { @@ -970,35 +972,14 @@ export function EmailComposer({ const sendMenuRef = useRef(null); const mobileSendMenuRef = useRef(null); - const saveTemplateModalRef = useFocusTrap({ - isActive: showSaveAsTemplate, - onEscape: () => setShowSaveAsTemplate(false), - restoreFocus: true, - }); - - const closeDialogRef = useFocusTrap({ - isActive: showCloseDialog, - onEscape: () => dismissCloseDialog(), - restoreFocus: true, - }); - - const attachmentWarningRef = useFocusTrap({ - isActive: showAttachmentWarning, - onEscape: () => setShowAttachmentWarning(false), - restoreFocus: true, - }); - - const emptySubjectWarningRef = useFocusTrap({ - isActive: showEmptySubjectWarning, - onEscape: () => setShowEmptySubjectWarning(false), - restoreFocus: true, - }); - - const placeholderWarningRef = useFocusTrap({ - isActive: showPlaceholderWarning, - onEscape: () => setShowPlaceholderWarning(false), - restoreFocus: true, - }); + // Save as template: changes made in its form are asked about before it closes (R5). + const [saveTemplateDirty, setSaveTemplateDirty] = useState(false); + const { confirmDiscard: confirmDiscardTemplate, discardDialog: discardTemplateDialog } = useDiscardChanges(); + const closeSaveAsTemplate = async () => { + if (!(await confirmDiscardTemplate(saveTemplateDirty))) return; + setSaveTemplateDirty(false); + setShowSaveAsTemplate(false); + }; const { client: activeClient } = useAuthStore(); const composerOwnClientRef = useRef(activeClient); @@ -3861,11 +3842,8 @@ export function EmailComposer({ ) : ( -
-
- +
+

{mode === 'reply' ? t('reply') : mode === 'replyAll' ? t('reply_all') : mode === 'forward' ? t('forward') : t('new_message')} @@ -3952,6 +3930,16 @@ export function EmailComposer({ {t('send')} )} + {/* Close at the top end (R4). On a phone it moves to the start, as + the phone's mail apps have it, with Send at the end. */} + handleClose()} + className="-me-2 max-md:order-first max-md:me-0 max-md:-ms-2 max-md:h-11 max-md:w-11" + label={tCommon('close')} + data-testid="composer-close" + > + +

)} @@ -4709,14 +4697,15 @@ export function EmailComposer({ )} {showSaveAsTemplate && ( -
-
-

{t('save_as_template')}

+ void closeSaveAsTemplate()} + title={t('save_as_template')} + size="lg" + dirty={saveTemplateDirty} + testId="save-as-template" + > +
{ addTemplate(data); + setSaveTemplateDirty(false); setShowSaveAsTemplate(false); }} - onCancel={() => setShowSaveAsTemplate(false)} + onCancel={() => void closeSaveAsTemplate()} + onDirtyChange={setSaveTemplateDirty} />
-
+ )} + {discardTemplateDialog} - {showAttachmentWarning && ( -
setShowAttachmentWarning(false)} - > -
e.stopPropagation()} - className="bg-background border border-border rounded-lg shadow-xl w-full max-w-md animate-in zoom-in-95 duration-200" - > -
-

{t('forgot_attachment.title')}

-

- {t('forgot_attachment.message', { keyword: attachmentWarningKeyword })} -

-
-
- - -
-
-
- )} + setShowAttachmentWarning(false)} + role="alertdialog" + showClose={false} + title={t('forgot_attachment.title')} + description={t('forgot_attachment.message', { keyword: attachmentWarningKeyword })} + footer={ + <> + + + + } + /> - {showEmptySubjectWarning && ( -
setShowEmptySubjectWarning(false)} - > -
e.stopPropagation()} - className="bg-background border border-border rounded-lg shadow-xl w-full max-w-md animate-in zoom-in-95 duration-200" - > -
-

{t('empty_subject.title')}

-

{t('empty_subject.message')}

- -
-
+ setShowEmptySubjectWarning(false)} + role="alertdialog" + showClose={false} + title={t('empty_subject.title')} + description={t('empty_subject.message')} + footer={ + <> -
-
-
- )} + + } + > + + - {showPlaceholderWarning && ( -
setShowPlaceholderWarning(false)} - > -
e.stopPropagation()} - className="bg-background border border-border rounded-lg shadow-xl w-full max-w-md animate-in zoom-in-95 duration-200" - > -
-

{t('placeholder_warning.title')}

-

{t('placeholder_warning.message')}

-
    - {placeholderWarningMatches.slice(0, 5).map((p) => ( -
  • {p}
    • - ))} - {placeholderWarningMatches.length > 5 && ( -
  • - {t('placeholder_warning.more', { count: placeholderWarningMatches.length - 5 })} -
    • - )} -
- -
-
+ setShowPlaceholderWarning(false)} + role="alertdialog" + showClose={false} + title={t('placeholder_warning.title')} + description={t('placeholder_warning.message')} + footer={ + <> -
-
-
- )} + + } + > +
    + {placeholderWarningMatches.slice(0, 5).map((p) => ( +
  • {p}
    • + ))} + {placeholderWarningMatches.length > 5 && ( +
  • + {t('placeholder_warning.more', { count: placeholderWarningMatches.length - 5 })} +
    • + )} +
+ + - {showCloseDialog && ( -
dismissCloseDialog()} - > -
e.stopPropagation()} - className="bg-background border border-border rounded-lg shadow-xl w-full max-w-md animate-in zoom-in-95 duration-200" - > -
-

- {closeDialogKind === 'discard' ? t('discard_draft_title') : t('close_draft_title')} -

-

- {closeDialogKind === 'discard' ? t('discard_draft_message') : t('close_draft_message')} -

-
-
- + + {closeDialogKind === 'close' && ( + - - {closeDialogKind === 'close' && ( - - )} -
-
-
- )} + )} + + } + /> {previewAttachment && ( { + await Promise.resolve(); + }); +} + +describe('TemplateManagerModal', () => { + beforeEach(() => { + useTemplateStore.setState({ templates: [template] } as never); + }); + + it('has the Dialog close X and closes on it when nothing was changed', async () => { + const onClose = vi.fn(); + render(); + const dialog = screen.getByRole('dialog', { name: 'title' }); + fireEvent.click(screen.getByRole('button', { name: 'close' })); + await flush(); + expect(onClose).toHaveBeenCalledTimes(1); + expect(dialog).toBeTruthy(); + }); + + it('asks before the X drops a template being written, and keeps it on Keep editing', async () => { + const onClose = vi.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: 'add' })); + fireEvent.change(screen.getByPlaceholderText('name_placeholder'), { target: { value: 'New one' } }); + + fireEvent.click(screen.getByRole('button', { name: 'close' })); + await flush(); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByRole('alertdialog', { name: 'discard_changes' })).toBeTruthy(); + + fireEvent.click(screen.getByRole('button', { name: 'keep_editing' })); + await flush(); + expect(onClose).not.toHaveBeenCalled(); + expect((screen.getByPlaceholderText('name_placeholder') as HTMLInputElement).value).toBe('New one'); + }); + + it('ignores a click outside while the form holds changes', async () => { + const onClose = vi.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: 'add' })); + fireEvent.change(screen.getByPlaceholderText('name_placeholder'), { target: { value: 'x' } }); + const backdrop = screen.getByTestId('template-manager').parentElement!; + fireEvent.mouseDown(backdrop); + fireEvent.click(backdrop); + await flush(); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.queryByRole('alertdialog')).toBeNull(); + }); + + it('asks before deleting a template instead of an inline Delete strip', async () => { + render(); + fireEvent.click(screen.getByRole('button', { name: 'delete' })); + await flush(); + const confirm = screen.getByRole('alertdialog', { name: 'delete_title' }); + expect(useTemplateStore.getState().templates).toHaveLength(1); + fireEvent.click(confirm.querySelector('button.bg-destructive') as HTMLElement); + await flush(); + expect(useTemplateStore.getState().templates).toHaveLength(0); + }); + + it('names its row buttons', () => { + render(); + expect(screen.getByRole('button', { name: 'edit' })).toBeTruthy(); + expect(screen.getByRole('button', { name: 'duplicate' })).toBeTruthy(); + expect(screen.getByRole('button', { name: 'favorite' })).toBeTruthy(); + }); +}); + +describe('PlaceholderFillModal', () => { + function renderFill(onClose = vi.fn()) { + render( + , + ); + return onClose; + } + + it('closes on Escape at once while nothing was typed', async () => { + const onClose = renderFill(); + fireEvent.keyDown(screen.getByRole('dialog', { name: 'fill_placeholders' }), { key: 'Escape' }); + await flush(); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('asks before Escape drops typed values', async () => { + const onClose = renderFill(); + fireEvent.change(screen.getByPlaceholderText('enter_value'), { target: { value: 'Ada' } }); + fireEvent.keyDown(screen.getByRole('dialog', { name: 'fill_placeholders' }), { key: 'Escape' }); + await flush(); + expect(onClose).not.toHaveBeenCalled(); + fireEvent.click(screen.getByRole('button', { name: 'discard' })); + await flush(); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('has an outline secondary button and the close X', () => { + renderFill(); + expect(screen.getByRole('button', { name: 'insert_raw' }).className).toContain('border-input'); + expect(screen.getByRole('button', { name: 'close' })).toBeTruthy(); + }); +}); diff --git a/components/templates/placeholder-fill-modal.tsx b/components/templates/placeholder-fill-modal.tsx index e66ad426c..3dd5e8d2e 100644 --- a/components/templates/placeholder-fill-modal.tsx +++ b/components/templates/placeholder-fill-modal.tsx @@ -2,13 +2,12 @@ import { useState, useMemo } from 'react'; import { useTranslations } from 'next-intl'; -import { X } from '@/components/icons'; -import { cn } from '@/lib/utils'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; +import { Dialog } from '@/components/ui/dialog'; +import { useDiscardChanges } from '@/components/settings/use-discard-changes'; import { substitutePlaceholders, isBuiltInPlaceholder } from '@/lib/template-utils'; import { sanitizeEmailHtml } from '@/lib/email-sanitization'; -import { useFocusTrap } from '@/hooks/use-focus-trap'; import type { EmailTemplate } from '@/lib/template-types'; interface PlaceholderFillModalProps { @@ -29,60 +28,62 @@ export function PlaceholderFillModal({ onClose, }: PlaceholderFillModalProps) { const t = useTranslations('templates'); + const { confirmDiscard, discardDialog } = useDiscardChanges(); - const [values, setValues] = useState>(() => { + const [initialValues] = useState>(() => { const initial: Record = {}; for (const p of placeholders) { initial[p] = autoFilled[p] || ''; } return initial; }); - - const modalRef = useFocusTrap({ - isActive: true, - onEscape: onClose, - restoreFocus: true, - }); + const [values, setValues] = useState>(initialValues); + // Typed values would be lost: Escape, the X and outside clicks ask first (R5). + const dirty = placeholders.some((p) => values[p] !== initialValues[p]); const preview = useMemo(() => { return substitutePlaceholders(template.body, values); }, [template.body, values]); - return ( -
-
-
-

- {t('fill_placeholders')} -

- -
+ const requestClose = async () => { + if (await confirmDiscard(dirty)) onClose(); + }; -
+ return ( + <> + void requestClose()} + title={t('fill_placeholders')} + dirty={dirty} + size="md" + testId="placeholder-fill" + onSubmit={(e) => { + e.preventDefault(); + onConfirm(values); + }} + footer={ + <> + + + + } + > +
{placeholders.map((p) => (
-
+
+ {discardDialog} + ); } diff --git a/components/templates/template-form.tsx b/components/templates/template-form.tsx index b122ef3fd..794c0cca6 100644 --- a/components/templates/template-form.tsx +++ b/components/templates/template-form.tsx @@ -1,10 +1,12 @@ 'use client'; -import { useState, useMemo } from 'react'; +import { useState, useMemo, useEffect, useRef } from 'react'; import { useTranslations } from 'next-intl'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; import { Star, Plus, Square, CheckSquare } from '@/components/icons'; +import { ContextMenu, ContextMenuItem } from '@/components/ui/context-menu'; +import { useContextMenu } from '@/hooks/use-context-menu'; import { cn } from '@/lib/utils'; import { validateTemplateName } from '@/lib/template-utils'; import { BUILT_IN_PLACEHOLDERS } from '@/lib/template-types'; @@ -24,9 +26,11 @@ interface TemplateFormProps { }; onSave: (data: Omit) => void; onCancel: () => void; + /** Told whenever the form gets or loses changes, so its host can ask before closing (R5). */ + onDirtyChange?: (dirty: boolean) => void; } -export function TemplateForm({ template, initialData, onSave, onCancel }: TemplateFormProps) { +export function TemplateForm({ template, initialData, onSave, onCancel, onDirtyChange }: TemplateFormProps) { const t = useTranslations('templates'); const tSettings = useTranslations('settings.templates'); const tComposer = useTranslations('email_composer'); @@ -51,7 +55,19 @@ export function TemplateForm({ template, initialData, onSave, onCancel }: Templa const [identityId, setIdentityId] = useState(template?.identityId || ''); const [isFavorite, setIsFavorite] = useState(template?.isFavorite || false); const [nameError, setNameError] = useState(null); - const [showPlaceholderMenu, setShowPlaceholderMenu] = useState<'subject' | 'body' | null>(null); + const placeholderMenu = useContextMenu<'subject' | 'body'>(); + + // Changed from what the form opened with. + const snapshot = JSON.stringify([name, category, subject, body, isHTML, toRecipients, ccRecipients, bccRecipients, identityId, isFavorite]); + const [initialSnapshot] = useState(snapshot); + const dirty = snapshot !== initialSnapshot; + const onDirtyChangeRef = useRef(onDirtyChange); + useEffect(() => { + onDirtyChangeRef.current = onDirtyChange; + }); + useEffect(() => { + onDirtyChangeRef.current?.(dirty); + }, [dirty]); const existingCategories = useMemo(() => { const cats = new Set(templates.map((t) => t.category).filter(Boolean)); @@ -94,9 +110,22 @@ export function TemplateForm({ template, initialData, onSave, onCancel }: Templa } else { setBody((prev) => prev + tag); } - setShowPlaceholderMenu(null); }; + const placeholderButton = (field: 'subject' | 'body') => ( + + ); + return (
@@ -105,7 +134,7 @@ export function TemplateForm({ template, initialData, onSave, onCancel }: Templa value={name} onChange={(e) => { setName(e.target.value); setNameError(null); }} placeholder={tSettings('name_placeholder')} - className={cn('mt-1', nameError && 'border-red-500')} + className={cn('mt-1', nameError && 'border-destructive')} autoFocus /> {nameError && ( @@ -136,24 +165,7 @@ export function TemplateForm({ template, initialData, onSave, onCancel }: Templa
-
- - {showPlaceholderMenu === 'subject' && ( - insertPlaceholder(p, 'subject')} - onClose={() => setShowPlaceholderMenu(null)} - /> - )} -
+ {placeholderButton('subject')}
-
- - {showPlaceholderMenu === 'body' && ( - insertPlaceholder(p, 'body')} - onClose={() => setShowPlaceholderMenu(null)} - /> - )} -
+ {placeholderButton('body')}