fix(web): avoid nesting buttons inside links (#11425)

2025-10-31 10:49:11 -04:00 · 2024-07-29 16:36:10 +02:00 · 2024-07-29 16:36:10 +02:00 · 2e059bfbfd
commit 2e059bfbfd
parent 7bb7f63d57
15 changed files with 216 additions and 96 deletions
--- a/e2e/src/web/specs/auth.e2e-spec.ts
+++ b/e2e/src/web/specs/auth.e2e-spec.ts
@ -13,7 +13,7 @@ test.describe('Registration', () => {
  test('admin registration', async ({ page }) => {
    // welcome
    await page.goto('/');
-    await page.getByRole('button', { name: 'Getting Started' }).click();
+    await page.getByRole('link', { name: 'Getting Started' }).click();

    // register
    await expect(page).toHaveTitle(/Admin Registration/);
--- a/web/src/lib/components/elements/buttons/test/button.spec.ts
+++ b/web/src/lib/components/elements/buttons/test/button.spec.ts
@ -0,0 +1,20 @@
+import Button from '$lib/components/elements/buttons/button.svelte';
+import { render, screen } from '@testing-library/svelte';
+
+describe('Button component', () => {
+  it('should render as a button', () => {
+    render(Button);
+    const button = screen.getByRole('button');
+    expect(button).toBeInTheDocument();
+    expect(button).toHaveAttribute('type', 'button');
+    expect(button).not.toHaveAttribute('href');
+  });
+
+  it('should render as a link if href prop is set', () => {
+    render(Button, { props: { href: '/test' } });
+    const link = screen.getByRole('link');
+    expect(link).toBeInTheDocument();
+    expect(link).toHaveAttribute('href', '/test');
+    expect(link).not.toHaveAttribute('type');
+  });
+});
--- a/web/src/lib/components/elements/buttons/test/circle-icon-button.spec.ts
+++ b/web/src/lib/components/elements/buttons/test/circle-icon-button.spec.ts
@ -0,0 +1,29 @@
+import CircleIconButton from '$lib/components/elements/buttons/circle-icon-button.svelte';
+import { render, screen } from '@testing-library/svelte';
+
+describe('CircleIconButton component', () => {
+  it('should render as a button', () => {
+    render(CircleIconButton, { icon: '', title: 'test' });
+    const button = screen.getByRole('button');
+    expect(button).toBeInTheDocument();
+    expect(button).toHaveAttribute('type', 'button');
+    expect(button).not.toHaveAttribute('href');
+    expect(button).toHaveAttribute('title', 'test');
+  });
+
+  it('should render as a link if href prop is set', () => {
+    render(CircleIconButton, { props: { href: '/test', icon: '', title: 'test' } });
+    const link = screen.getByRole('link');
+    expect(link).toBeInTheDocument();
+    expect(link).toHaveAttribute('href', '/test');
+    expect(link).not.toHaveAttribute('type');
+  });
+
+  it('should render icon inside button', () => {
+    render(CircleIconButton, { icon: '', title: 'test' });
+    const button = screen.getByRole('button');
+    const icon = button.querySelector('svg');
+    expect(icon).toBeInTheDocument();
+    expect(icon).toHaveAttribute('aria-label', 'test');
+  });
+});
--- a/web/src/lib/components/elements/buttons/button.svelte
+++ b/web/src/lib/components/elements/buttons/button.svelte
@ -1,5 +1,6 @@
 <script lang="ts" context="module">
-  export type Type = 'button' | 'submit' | 'reset';
+  import type { HTMLButtonAttributes, HTMLLinkAttributes } from 'svelte/elements';
+
  export type Color =
    | 'primary'
    | 'primary-inversed'
@ -14,45 +15,66 @@
    | 'dark-gray'
    | 'overlay-primary';
  export type Size = 'tiny' | 'icon' | 'link' | 'sm' | 'base' | 'lg';
-  export type Rounded = 'lg' | '3xl' | 'full' | false;
+  export type Rounded = 'lg' | '3xl' | 'full' | 'none';
  export type Shadow = 'md' | false;
+
+  type BaseProps = {
+    class?: string;
+    color?: Color;
+    size?: Size;
+    rounded?: Rounded;
+    shadow?: Shadow;
+    fullwidth?: boolean;
+    border?: boolean;
+  };
+
+  export type ButtonProps = HTMLButtonAttributes &
+    BaseProps & {
+      href?: never;
+    };
+
+  export type LinkProps = HTMLLinkAttributes &
+    BaseProps & {
+      type?: never;
+    };
+
+  export type Props = ButtonProps | LinkProps;
 </script>

 <script lang="ts">
-  export let type: Type = 'button';
+  type $$Props = Props;
+
+  export let type: $$Props['type'] = 'button';
+  export let href: $$Props['href'] = undefined;
  export let color: Color = 'primary';
  export let size: Size = 'base';
  export let rounded: Rounded = '3xl';
  export let shadow: Shadow = 'md';
-  export let disabled = false;
  export let fullwidth = false;
  export let border = false;
-  export let title: string | undefined = '';
-  export let form: string | undefined = undefined;

  let className = '';
  export { className as class };

  const colorClasses: Record<Color, string> = {
    primary:
-      'bg-immich-primary dark:bg-immich-dark-primary text-white dark:text-immich-dark-gray enabled:dark:hover:bg-immich-dark-primary/80 enabled:hover:bg-immich-primary/90',
+      'bg-immich-primary dark:bg-immich-dark-primary text-white dark:text-immich-dark-gray dark:hover:bg-immich-dark-primary/80 hover:bg-immich-primary/90',
    secondary:
-      'bg-gray-500 dark:bg-gray-200 text-white dark:text-immich-dark-gray enabled:hover:bg-gray-500/90 enabled:dark:hover:bg-gray-200/90',
-    'transparent-primary':
-      'text-gray-500 dark:text-immich-dark-primary enabled:hover:bg-gray-100 enabled:dark:hover:bg-gray-700',
+      'bg-gray-500 dark:bg-gray-200 text-white dark:text-immich-dark-gray hover:bg-gray-500/90 dark:hover:bg-gray-200/90',
+    'transparent-primary': 'text-gray-500 dark:text-immich-dark-primary hover:bg-gray-100 dark:hover:bg-gray-700',
    'text-primary':
-      'text-immich-primary dark:text-immich-dark-primary enabled:dark:hover:bg-immich-dark-primary/10 enabled:hover:bg-immich-primary/10',
-    'light-red': 'bg-[#F9DEDC] text-[#410E0B] enabled:hover:bg-red-50',
-    red: 'bg-red-500 text-white enabled:hover:bg-red-400',
-    green: 'bg-green-400 text-gray-800 enabled:hover:bg-green-400/90',
-    gray: 'bg-gray-500 dark:bg-gray-200 enabled:hover:bg-gray-500/75 enabled:dark:hover:bg-gray-200/80 text-white dark:text-immich-dark-gray',
+      'text-immich-primary dark:text-immich-dark-primary dark:hover:bg-immich-dark-primary/10 hover:bg-immich-primary/10',
+    'light-red': 'bg-[#F9DEDC] text-[#410E0B] hover:bg-red-50',
+    red: 'bg-red-500 text-white hover:bg-red-400',
+    green: 'bg-green-400 text-gray-800 hover:bg-green-400/90',
+    gray: 'bg-gray-500 dark:bg-gray-200 hover:bg-gray-500/75 dark:hover:bg-gray-200/80 text-white dark:text-immich-dark-gray',
    'transparent-gray':
-      'dark:text-immich-dark-fg enabled:hover:bg-immich-primary/5 enabled:hover:text-gray-700 enabled:hover:dark:text-immich-dark-fg enabled:dark:hover:bg-immich-dark-primary/25',
+      'dark:text-immich-dark-fg hover:bg-immich-primary/5 hover:text-gray-700 hover:dark:text-immich-dark-fg dark:hover:bg-immich-dark-primary/25',
    'dark-gray':
-      'dark:border-immich-dark-gray dark:bg-gray-500 enabled:dark:hover:bg-immich-dark-primary/50 enabled:hover:bg-immich-primary/10 dark:text-white',
-    'overlay-primary': 'text-gray-500 enabled:hover:bg-gray-100',
+      'dark:border-immich-dark-gray dark:bg-gray-500 dark:hover:bg-immich-dark-primary/50 hover:bg-immich-primary/10 dark:text-white',
+    'overlay-primary': 'text-gray-500 hover:bg-gray-100',
    'primary-inversed':
-      'bg-immich-dark-primary dark:bg-immich-primary text-black dark:text-white enabled:hover:bg-immich-dark-primary/80 enabled:dark:hover:bg-immich-primary/90',
+      'bg-immich-dark-primary dark:bg-immich-primary text-black dark:text-white hover:bg-immich-dark-primary/80 dark:hover:bg-immich-primary/90',
  };

  const sizeClasses: Record<Size, string> = {
@ -63,25 +85,37 @@
    base: 'px-6 py-3 font-medium',
    lg: 'px-6 py-4 font-semibold',
  };
+
+  const roundedClasses: Record<Rounded, string> = {
+    none: '',
+    lg: 'rounded-lg',
+    '3xl': 'rounded-3xl',
+    full: 'rounded-full',
+  };
+
+  $: computedClass = [
+    className,
+    colorClasses[color],
+    sizeClasses[size],
+    roundedClasses[rounded],
+    shadow === 'md' && 'shadow-md',
+    fullwidth && 'w-full',
+    border && 'border',
+  ]
+    .filter(Boolean)
+    .join(' ');
 </script>

-<button
-  {type}
-  {disabled}
-  {title}
-  {form}
+<!-- svelte-ignore a11y-no-static-element-interactions -->
+<svelte:element
+  this={href ? 'a' : 'button'}
+  type={href ? undefined : type}
+  {href}
  on:click
  on:focus
  on:blur
-  class="{className} inline-flex items-center justify-center transition-colors disabled:cursor-not-allowed disabled:opacity-60 {colorClasses[
-    color
-  ]} {sizeClasses[size]}"
-  class:rounded-lg={rounded === 'lg'}
-  class:rounded-3xl={rounded === '3xl'}
-  class:rounded-full={rounded === 'full'}
-  class:shadow-md={shadow === 'md'}
-  class:w-full={fullwidth}
-  class:border
+  class="inline-flex items-center justify-center transition-colors disabled:cursor-not-allowed disabled:opacity-60 disabled:pointer-events-none {computedClass}"
+  {...$$restProps}
 >
  <slot />
-</button>
+</svelte:element>
--- a/web/src/lib/components/elements/buttons/circle-icon-button.svelte
+++ b/web/src/lib/components/elements/buttons/circle-icon-button.svelte
@ -1,18 +1,48 @@
 <script lang="ts" context="module">
+  import type { HTMLButtonAttributes, HTMLLinkAttributes } from 'svelte/elements';
+
  export type Color = 'transparent' | 'light' | 'dark' | 'gray' | 'primary' | 'opaque';
+  export type Padding = '1' | '2' | '3';
+
+  type BaseProps = {
+    icon: string;
+    title: string;
+    class?: string;
+    color?: Color;
+    padding?: Padding;
+    size?: string;
+    hideMobile?: true;
+    buttonSize?: string;
+    viewBox?: string;
+  };
+
+  export type ButtonProps = HTMLButtonAttributes &
+    BaseProps & {
+      href?: never;
+    };
+
+  export type LinkProps = HTMLLinkAttributes &
+    BaseProps & {
+      type?: never;
+    };
+
+  export type Props = ButtonProps | LinkProps;
 </script>

 <script lang="ts">
  import Icon from '$lib/components/elements/icon.svelte';

-  export let type: 'button' | 'submit' | 'reset' = 'button';
+  type $$Props = Props;
+
+  export let type: $$Props['type'] = 'button';
+  export let href: $$Props['href'] = undefined;
  export let icon: string;
  export let color: Color = 'transparent';
  export let title: string;
  /**
   * The padding of the button, used by the `p-{padding}` Tailwind CSS class.
   */
-  export let padding = '3';
+  export let padding: Padding = '3';
  /**
   * Size of the button, used for a CSS value.
   */
@ -23,12 +53,6 @@
   * viewBox attribute for the SVG icon.
   */
  export let viewBox: string | undefined = undefined;
-  export let id: string | undefined = undefined;
-  export let ariaHasPopup: boolean | undefined = undefined;
-  export let ariaExpanded: boolean | undefined = undefined;
-  export let ariaControls: string | undefined = undefined;
-  export let tabindex: number | undefined = undefined;
-  export let disabled: boolean | undefined = undefined;

  /**
   * Override the default styling of the button for specific use cases, such as the icon color.
@ -46,24 +70,28 @@
      'bg-immich-primary dark:bg-immich-dark-primary hover:bg-immich-primary/75 hover:dark:bg-immich-dark-primary/80 text-white dark:text-immich-dark-gray',
  };

+  const paddingClasses: Record<Padding, string> = {
+    '1': 'p-1',
+    '2': 'p-2',
+    '3': 'p-3',
+  };
+
  $: colorClass = colorClasses[color];
  $: mobileClass = hideMobile ? 'hidden sm:flex' : '';
-  $: paddingClass = `p-${padding}`;
+  $: paddingClass = paddingClasses[padding];
 </script>

-<button
-  {id}
+<!-- svelte-ignore a11y-no-static-element-interactions -->
+<svelte:element
+  this={href ? 'a' : 'button'}
+  type={href ? undefined : type}
  {title}
-  {type}
-  {tabindex}
-  {disabled}
+  {href}
  style:width={buttonSize ? buttonSize + 'px' : ''}
  style:height={buttonSize ? buttonSize + 'px' : ''}
  class="flex place-content-center place-items-center rounded-full {colorClass} {paddingClass} transition-all disabled:cursor-default hover:dark:text-immich-dark-gray {className} {mobileClass}"
-  aria-haspopup={ariaHasPopup}
-  aria-expanded={ariaExpanded}
-  aria-controls={ariaControls}
  on:click
+  {...$$restProps}
 >
  <Icon path={icon} {size} ariaLabel={title} {viewBox} color="currentColor" />
-</button>
+</svelte:element>
--- a/web/src/lib/components/elements/buttons/link-button.svelte
+++ b/web/src/lib/components/elements/buttons/link-button.svelte
@ -1,16 +1,22 @@
 <script lang="ts" context="module">
  export type Color = 'transparent-primary' | 'transparent-gray';
+
+  type BaseProps = {
+    color?: Color;
+  };
+
+  export type Props = (LinkProps & BaseProps) | (ButtonProps & BaseProps);
 </script>

 <script lang="ts">
-  import Button from './button.svelte';
+  import Button, { type ButtonProps, type LinkProps } from '$lib/components/elements/buttons/button.svelte';
+
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  type $$Props = Props;

  export let color: Color = 'transparent-gray';
-  export let disabled = false;
-  export let fullwidth = false;
-  export let title: string | undefined = undefined;
 </script>

-<Button {title} size="link" {color} shadow={false} rounded="lg" {disabled} on:click {fullwidth}>
+<Button size="link" {color} shadow={false} rounded="lg" on:click {...$$restProps}>
  <slot />
 </Button>
--- a/web/src/lib/components/elements/buttons/skip-link.svelte
+++ b/web/src/lib/components/elements/buttons/skip-link.svelte
@ -17,7 +17,7 @@
 <div class="absolute z-50 top-2 left-2 transition-transform {isFocused ? 'translate-y-0' : '-translate-y-10 sr-only'}">
  <Button
    size={'sm'}
-    rounded={false}
+    rounded="none"
    on:click={moveFocus}
    on:focus={() => (isFocused = true)}
    on:blur={() => (isFocused = false)}
--- a/web/src/lib/components/shared-components/context-menu/button-context-menu.svelte
+++ b/web/src/lib/components/shared-components/context-menu/button-context-menu.svelte
@ -1,5 +1,8 @@
 <script lang="ts">
-  import CircleIconButton, { type Color } from '$lib/components/elements/buttons/circle-icon-button.svelte';
+  import CircleIconButton, {
+    type Color,
+    type Padding,
+  } from '$lib/components/elements/buttons/circle-icon-button.svelte';
  import ContextMenu from '$lib/components/shared-components/context-menu/context-menu.svelte';
  import {
    getContextMenuPositionFromBoundingRect,
@ -24,7 +27,7 @@
  export let direction: 'left' | 'right' = 'right';
  export let color: Color = 'transparent';
  export let size: string | undefined = undefined;
-  export let padding: string | undefined = undefined;
+  export let padding: Padding | undefined = undefined;
  /**
   * Additional classes to apply to the button.
   */
@ -114,9 +117,9 @@
      {padding}
      {size}
      {title}
-      ariaControls={menuId}
-      ariaExpanded={isOpen}
-      ariaHasPopup={true}
+      aria-controls={menuId}
+      aria-expanded={isOpen}
+      aria-haspopup={true}
      class={buttonClass}
      id={buttonId}
      on:click={handleClick}
--- a/web/src/lib/components/shared-components/navigation-bar/account-info-panel.svelte
+++ b/web/src/lib/components/shared-components/navigation-bar/account-info-panel.svelte
@ -73,14 +73,19 @@
      <p class="text-sm text-gray-500 dark:text-immich-dark-fg">{$user.email}</p>
    </div>

-    <a href={AppRoute.USER_SETTINGS} on:click={() => dispatch('close')}>
-      <Button color="dark-gray" size="sm" shadow={false} border>
-        <div class="flex place-content-center place-items-center gap-2 px-2">
-          <Icon path={mdiCog} size="18" />
-          {$t('account_settings')}
-        </div>
-      </Button>
-    </a>
+    <Button
+      href={AppRoute.USER_SETTINGS}
+      on:click={() => dispatch('close')}
+      color="dark-gray"
+      size="sm"
+      shadow={false}
+      border
+    >
+      <div class="flex place-content-center place-items-center gap-2 px-2">
+        <Icon path={mdiCog} size="18" />
+        {$t('account_settings')}
+      </div>
+    </Button>
  </div>

  <div class="mb-4 flex flex-col">
--- a/web/src/lib/components/shared-components/navigation-bar/navigation-bar.svelte
+++ b/web/src/lib/components/shared-components/navigation-bar/navigation-bar.svelte
@ -60,9 +60,13 @@

      <section class="flex place-items-center justify-end gap-4 max-sm:w-full">
        {#if $featureFlags.search}
-          <a href={AppRoute.SEARCH} id="search-button" class="ml-4 sm:hidden">
-            <CircleIconButton title={$t('go_to_search')} icon={mdiMagnify} />
-          </a>
+          <CircleIconButton
+            href={AppRoute.SEARCH}
+            id="search-button"
+            class="ml-4 sm:hidden"
+            title={$t('go_to_search')}
+            icon={mdiMagnify}
+          />
        {/if}

        <ThemeButton />
--- a/web/src/lib/components/shared-components/purchasing/individual-purchase-option-card.svelte
+++ b/web/src/lib/components/shared-components/purchasing/individual-purchase-option-card.svelte
@ -37,8 +37,6 @@
      </div>
    </div>

-    <a href={getProductLink(ImmichProduct.Client)}>
-      <Button fullwidth>{$t('purchase_button_select')}</Button>
-    </a>
+    <Button href={getProductLink(ImmichProduct.Client)} fullwidth>{$t('purchase_button_select')}</Button>
  </div>
 </div>
--- a/web/src/lib/components/shared-components/purchasing/server-purchase-option-card.svelte
+++ b/web/src/lib/components/shared-components/purchasing/server-purchase-option-card.svelte
@ -37,8 +37,6 @@
      </div>
    </div>

-    <a href={getLicenseLink(ImmichProduct.Server)}>
-      <Button fullwidth>{$t('purchase_button_select')}</Button>
-    </a>
+    <Button href={getLicenseLink(ImmichProduct.Server)} fullwidth>{$t('purchase_button_select')}</Button>
  </div>
 </div>
--- a/web/src/routes/(user)/sharing/+page.svelte
+++ b/web/src/routes/(user)/sharing/+page.svelte
@ -1,5 +1,4 @@
 <script lang="ts">
-  import { goto } from '$app/navigation';
  import empty2Url from '$lib/assets/empty-2.svg';
  import LinkButton from '$lib/components/elements/buttons/link-button.svelte';
  import Icon from '$lib/components/elements/icon.svelte';
@ -43,7 +42,7 @@
      </div>
    </LinkButton>

-    <LinkButton on:click={() => goto(AppRoute.SHARED_LINKS)}>
+    <LinkButton href={AppRoute.SHARED_LINKS}>
      <div class="flex flex-wrap place-items-center justify-center gap-x-1 text-sm">
        <Icon path={mdiLink} size="18" class="shrink-0" />
        <span class="leading-none max-sm:text-xs">{$t('shared_links')}</span>
--- a/web/src/routes/+page.svelte
+++ b/web/src/routes/+page.svelte
@ -11,10 +11,8 @@
      <ImmichLogo noText class="text-center" height="200" width="200" />
    </div>
    <h1 class="text-4xl font-bold text-immich-primary dark:text-immich-dark-primary">{$t('welcome_to_immich')}</h1>
-    <a href={AppRoute.AUTH_REGISTER}>
-      <Button size="lg" rounded="lg">
-        <span class="px-2 font-bold">{$t('getting_started')}</span>
-      </Button>
-    </a>
+    <Button href={AppRoute.AUTH_REGISTER} size="lg" rounded="lg">
+      <span class="px-2 font-bold">{$t('getting_started')}</span>
+    </Button>
  </div>
 </section>
--- a/web/src/routes/admin/jobs-status/+page.svelte
+++ b/web/src/routes/admin/jobs-status/+page.svelte
@ -31,14 +31,12 @@

 <UserPageLayout title={data.meta.title} admin>
  <div class="flex justify-end" slot="buttons">
-    <a href="{AppRoute.ADMIN_SETTINGS}?isOpen=job">
-      <LinkButton>
-        <div class="flex place-items-center gap-2 text-sm">
-          <Icon path={mdiCog} size="18" />
-          {$t('admin.manage_concurrency')}
-        </div>
-      </LinkButton>
-    </a>
+    <LinkButton href="{AppRoute.ADMIN_SETTINGS}?isOpen=job">
+      <div class="flex place-items-center gap-2 text-sm">
+        <Icon path={mdiCog} size="18" />
+        {$t('admin.manage_concurrency')}
+      </div>
+    </LinkButton>
  </div>
  <section id="setting-content" class="flex place-content-center sm:mx-4">
    <section class="w-full pb-28 sm:w-5/6 md:w-[850px]">