Merge pull request #1032 from douwezijlstra-frl/fix/hover-tag-quick-action

fix: open the tag picker from the labels hover quick-action
2026-11-01 16:12:02 +00:00 · 2026-09-23 19:16:55 +02:00
parent a4bc303f45 a91d79b732
commit 53dff446a2
2 changed files with 223 additions and 4 deletions
@@ -1,5 +1,7 @@
 "use client";

+import { useCallback, useEffect, useRef, useState } from "react";
+import { createPortal } from "react-dom";
 import { Email } from "@/lib/jmap/types";
 import { useSettingsStore } from "@/stores/settings-store";
 import type { HoverAction } from "@/stores/settings-store";
@@ -7,6 +9,8 @@ import { cn } from "@/lib/utils";
 import { Trash2, Star, Mail, MailOpen, Archive, Tag, ShieldAlert, ShieldCheck } from "@/components/icons";
 import { useTranslations } from "next-intl";
 import { useIsMobile } from "@/hooks/use-media-query";
+import { TagPicker } from "@/components/email/tag-picker";
+import { getEmailTagIds } from "@/lib/thread-utils";

 interface EmailHoverActionsProps {
  email: Email;
@@ -69,6 +73,10 @@ const CORNER_CLASSES = {
  'bottom-left': 'bottom-1 left-1',
 } as const;

+/** Roughly the picker's height; used to flip it above the row near the viewport bottom. */
+const TAG_PICKER_MAX_HEIGHT = 320;
+const TAG_PICKER_WIDTH = 224;
+
 export function EmailHoverActions({
  email,
  backgroundClassName = "bg-muted",
@@ -88,8 +96,48 @@ export function EmailHoverActions({
  const t = useTranslations("settings.email_behavior.hover_actions");
  const isMobile = useIsMobile();

+  // The tag quick-action opens the same picker the context menu and the reading
+  // pane use. It is portalled to the body because every row sits inside a
+  // `transform`ed virtual item, which would make a `fixed` child scroll with the
+  // list and clip it at the scroller's edge.
+  const [tagPickerPos, setTagPickerPos] = useState<{ top: number; left: number } | null>(null);
+  const pickerRef = useRef<HTMLDivElement | null>(null);
+  const tagButtonRef = useRef<HTMLElement | null>(null);
+  const closeTagPicker = useCallback(() => setTagPickerPos(null), []);
+
+  useEffect(() => {
+    if (!tagPickerPos) return;
+    const onPointerDown = (event: PointerEvent) => {
+      const target = event.target as Node;
+      // The anchor is left to its own click handler, which toggles the picker
+      // shut - closing here first would let that click reopen it.
+      if (tagButtonRef.current?.contains(target)) return;
+      if (!pickerRef.current?.contains(target)) closeTagPicker();
+    };
+    const onKeyDown = (event: KeyboardEvent) => {
+      if (event.key === "Escape") closeTagPicker();
+    };
+    // The anchor moves with the list, so scrolling dismisses rather than chases
+    // it - but the picker's own tag list scrolls too, and that must not close it.
+    const onScroll = (event: Event) => {
+      if (pickerRef.current?.contains(event.target as Node)) return;
+      closeTagPicker();
+    };
+    document.addEventListener("pointerdown", onPointerDown, true);
+    document.addEventListener("keydown", onKeyDown);
+    window.addEventListener("scroll", onScroll, true);
+    window.addEventListener("resize", closeTagPicker);
+    return () => {
+      document.removeEventListener("pointerdown", onPointerDown, true);
+      document.removeEventListener("keydown", onKeyDown);
+      window.removeEventListener("scroll", onScroll, true);
+      window.removeEventListener("resize", closeTagPicker);
+    };
+  }, [tagPickerPos, closeTagPicker]);
+
  const isUnread = !email.keywords?.$seen;
  const isStarred = email.keywords?.$flagged;
+  const currentTagIds = getEmailTagIds(email.keywords);
  const hoverBackgroundClassName = backgroundClassName;

  if (isMobile) return null;
@@ -111,9 +159,28 @@ export function EmailHoverActions({
      case "archive":
        onArchive?.();
        break;
-      case "tag":
-        onSetTag?.(null);
+      case "tag": {
+        // Previously this cleared every tag on the message, which is a no-op on
+        // an untagged one - hence "clicking it does nothing". Open the picker.
+        if (!onSetTag) break;
+        tagButtonRef.current = e.currentTarget as HTMLElement;
+        const rect = e.currentTarget.getBoundingClientRect();
+        setTagPickerPos((open) =>
+          open
+            ? null
+            : {
+                top:
+                  rect.bottom + TAG_PICKER_MAX_HEIGHT > window.innerHeight
+                    ? Math.max(8, rect.top - TAG_PICKER_MAX_HEIGHT - 4)
+                    : rect.bottom + 4,
+                left: Math.max(
+                  8,
+                  Math.min(rect.left, window.innerWidth - TAG_PICKER_WIDTH - 8),
+                ),
+              },
+        );
        break;
+      }
      case "spam":
        if (isInJunk) onUndoSpam?.();
        else onMarkAsSpam?.();
@@ -144,8 +211,13 @@ export function EmailHoverActions({
    return (
      <button
        key={actionId}
+        type="button"
+        data-testid={`hover-action-${actionId}`}
        onClick={(e) => handleAction(e, actionId)}
+        onDoubleClick={(e) => { e.stopPropagation(); e.preventDefault(); }}
        title={title}
+        aria-haspopup={actionId === "tag" ? "menu" : undefined}
+        aria-expanded={actionId === "tag" ? tagPickerPos !== null : undefined}
        className={cn(
          "p-1.5 rounded-md transition-colors duration-100 text-muted-foreground hover:bg-black/5 dark:hover:bg-white/10",
          className,
@@ -161,11 +233,37 @@ export function EmailHoverActions({
    );
  });

+  const tagPicker =
+    tagPickerPos && onSetTag
+      ? createPortal(
+          <div
+            ref={pickerRef}
+            data-testid="hover-tag-picker"
+            role="menu"
+            className="fixed z-50 w-56 max-w-[18rem] rounded-lg border border-border bg-popover py-1 shadow-lg"
+            style={{ top: tagPickerPos.top, left: tagPickerPos.left }}
+            onClick={(e) => e.stopPropagation()}
+            onContextMenu={(e) => e.stopPropagation()}
+          >
+            <TagPicker
+              selectedIds={currentTagIds}
+              onToggle={(tagId) => onSetTag(tagId)}
+            />
+          </div>,
+          document.body,
+        )
+      : null;
+
+  // While the picker is open the row is no longer hovered, so the toolbar has to
+  // stay mounted and visible or the picker would close under its own anchor.
+  const visibilityClass = tagPickerPos ? "flex" : "hidden group-hover:flex";
+
  if (hoverActionsMode === 'floating') {
    return (
      <div
        className={cn(
-          "absolute z-10 hidden group-hover:flex items-center",
+          "absolute z-10 items-center",
+          visibilityClass,
          CORNER_CLASSES[hoverActionsCorner],
        )}
      >
@@ -175,13 +273,14 @@ export function EmailHoverActions({
            {actionButtons}
          </div>
        </div>
+        {tagPicker}
      </div>
    );
  }

  return (
    <div
-      className="absolute end-0 top-0 bottom-0 z-10 hidden group-hover:flex items-center"
+      className={cn("absolute end-0 top-0 bottom-0 z-10 items-center", visibilityClass)}
    >
      <div
        className={cn(
@@ -198,6 +297,7 @@ export function EmailHoverActions({
          {actionButtons}
        </div>
      </div>
+      {tagPicker}
    </div>
  );
 }
@@ -0,0 +1,119 @@
+import { test, expect } from '@playwright/test';
+import { ACCOUNTS } from './helpers/config';
+import { sendMail } from './helpers/smtp';
+import { JmapClient } from './helpers/jmap';
+import { login, seedSettings, emailItem, expectEmailVisible } from './helpers/app';
+
+/**
+ * The Labels quick-action on a list row (Settings -> Appearance & layout ->
+ * Hover actions). It used to call `onSetTag(null)`, which clears every tag on
+ * the message - a no-op on an untagged one, so clicking it appeared to do
+ * nothing. It now opens the same tag picker the context menu and the reading
+ * pane use.
+ */
+const alice = ACCOUNTS.alice;
+let seq = 0;
+const subj = (l: string) => `IT ${l} ${Date.now()}-${seq++}`;
+const send = (subject: string) =>
+  sendMail({ from: alice.email, authPass: alice.password, to: alice.email, subject, body: 'x' });
+
+/** The row's tag button, made visible by hovering the row. */
+async function openHoverTagPicker(page: import('@playwright/test').Page, subject: string) {
+  const row = emailItem(page, subject).first();
+  await row.scrollIntoViewIfNeeded();
+  await row.hover();
+  const button = row.locator('[data-testid="hover-action-tag"]');
+  await expect(button).toBeVisible();
+  await button.click();
+  return page.locator('[data-testid="hover-tag-picker"]');
+}
+
+test.describe('Labels hover quick-action', () => {
+  let jmap: JmapClient;
+
+  test.beforeEach(async ({ page }) => {
+    jmap = await JmapClient.connect(alice.email, alice.password);
+    await jmap.reset();
+    // Only the tag quick-action, so the button position is unambiguous.
+    await seedSettings(page, { hoverActions: ['tag'] });
+  });
+
+  test('opens the tag picker and applies the tag to the message', async ({ page }) => {
+    const s = subj('hover-tag');
+    await send(s);
+    await jmap.waitForEmail(s);
+
+    await login(page, alice);
+    await expectEmailVisible(page, s);
+
+    const picker = await openHoverTagPicker(page, s);
+    await expect(picker).toBeVisible();
+
+    const red = picker.getByRole('menuitemcheckbox', { name: 'Red' });
+    await expect(red).toHaveAttribute('aria-checked', 'false');
+    await red.click();
+
+    // The tag reaches the server under the current $label: prefix.
+    await expect(async () => {
+      const mail = await jmap.findEmailBySubject(s);
+      expect(mail?.keywords?.['$label:red']).toBe(true);
+    }).toPass({ timeout: 15000 });
+
+    // ...and the picker reflects it without being reopened.
+    await expect(red).toHaveAttribute('aria-checked', 'true');
+  });
+
+  test('clicking the same tag again removes it', async ({ page }) => {
+    const s = subj('hover-untag');
+    await send(s);
+    await jmap.waitForEmail(s);
+
+    await login(page, alice);
+    await expectEmailVisible(page, s);
+
+    const picker = await openHoverTagPicker(page, s);
+    const red = picker.getByRole('menuitemcheckbox', { name: 'Red' });
+
+    await red.click();
+    await expect(red).toHaveAttribute('aria-checked', 'true');
+
+    await red.click();
+    await expect(red).toHaveAttribute('aria-checked', 'false');
+
+    await expect(async () => {
+      const mail = await jmap.findEmailBySubject(s);
+      expect(mail?.keywords?.['$label:red']).toBeFalsy();
+    }).toPass({ timeout: 15000 });
+  });
+
+  test('stays open when the pointer leaves the row, and closes on Escape', async ({ page }) => {
+    const s = subj('hover-tag-open');
+    await send(s);
+    await jmap.waitForEmail(s);
+
+    await login(page, alice);
+    await expectEmailVisible(page, s);
+
+    const picker = await openHoverTagPicker(page, s);
+    // Moving off the row un-hovers it; the toolbar (and so the picker's anchor)
+    // must not disappear underneath the open picker.
+    await page.mouse.move(5, 5);
+    await expect(picker).toBeVisible();
+
+    await page.keyboard.press('Escape');
+    await expect(picker).toHaveCount(0);
+  });
+
+  test('clicking the tag button does not open the email', async ({ page }) => {
+    const s = subj('hover-tag-noopen');
+    await send(s);
+    await jmap.waitForEmail(s);
+
+    await login(page, alice);
+    await expectEmailVisible(page, s);
+
+    await openHoverTagPicker(page, s);
+    // The reading pane stays closed - the click must not fall through to the row.
+    await expect(page.locator('[data-tour="email-viewer"]')).toHaveCount(0);
+  });
+});