Merge pull request 'Phone hit: repo-wide guard for bare IconButtons, PhoneHitInPlace for preview arrows, drop dead Editor.preview' (#141) from void/phone-hit-guard into dev

src/app/components/editor/Editor.preview.tsx

@@ -1,84 +0,0 @@
-import React, { useState } from 'react';
-import FocusTrap from 'focus-trap-react';
-import {
-  config,
-  Icon,
-  IconButton,
-  Icons,
-  Line,
-  Modal,
-  Overlay,
-  OverlayBackdrop,
-  OverlayCenter,
-} from 'folds';
-
-import { CustomEditor, useEditor } from './Editor';
-import { Toolbar } from './Toolbar';
-import { stopPropagation } from '../../utils/keyboard';
-
-export function EditorPreview() {
-  const [open, setOpen] = useState(false);
-  const editor = useEditor();
-  const [toolbar, setToolbar] = useState(false);
-
-  return (
-    <>
-      <IconButton variant="SurfaceVariant" onClick={() => setOpen(!open)}>
-        <Icon src={Icons.BlockQuote} />
-      </IconButton>
-      <Overlay open={open} backdrop={<OverlayBackdrop />}>
-        <OverlayCenter>
-          <FocusTrap
-            focusTrapOptions={{
-              initialFocus: false,
-              onDeactivate: () => setOpen(false),
-              clickOutsideDeactivates: true,
-              escapeDeactivates: stopPropagation,
-            }}
-          >
-            <Modal size="500">
-              <div style={{ padding: config.space.S400 }}>
-                <CustomEditor
-                  editor={editor}
-                  placeholder="Send a message..."
-                  before={
-                    <IconButton variant="SurfaceVariant" size="300" radii="300">
-                      <Icon src={Icons.PlusCircle} />
-                    </IconButton>
-                  }
-                  after={
-                    <>
-                      <IconButton
-                        variant="SurfaceVariant"
-                        size="300"
-                        radii="300"
-                        onClick={() => setToolbar(!toolbar)}
-                        aria-pressed={toolbar}
-                      >
-                        <Icon src={toolbar ? Icons.AlphabetUnderline : Icons.Alphabet} />
-                      </IconButton>
-                      <IconButton variant="SurfaceVariant" size="300" radii="300">
-                        <Icon src={Icons.Smile} />
-                      </IconButton>
-                      <IconButton variant="SurfaceVariant" size="300" radii="300">
-                        <Icon src={Icons.Send} />
-                      </IconButton>
-                    </>
-                  }
-                  bottom={
-                    toolbar && (
-                      <div>
-                        <Line variant="SurfaceVariant" size="300" />
-                        <Toolbar />
-                      </div>
-                    )
-                  }
-                />
-              </div>
-            </Modal>
-          </FocusTrap>
-        </OverlayCenter>
-      </Overlay>
-    </>
-  );
-}

src/app/components/url-preview/UrlPreviewCard.tsx

@@ -1,3 +1,4 @@
+import classNames from 'classnames';
 import React, { useCallback, useEffect, useRef, useState } from 'react';
 import { IPreviewUrlResponse } from 'matrix-js-sdk';
 import { Box, Icon, IconButton, Icons, Scroll, Spinner, Text, as, color, config } from 'folds';
@@ -10,6 +11,7 @@ import {
   useIntersectionObserver,
 } from '../../hooks/useIntersectionObserver';
 import * as css from './UrlPreviewCard.css';
+import { PhoneHitInPlace } from '../../styles/PhoneHit.css';
 import { tryDecodeURIComponent } from '../../utils/dom';
 import { mxcUrlToHttp } from '../../utils/matrix';
 import { useMediaAuthentication } from '../../hooks/useMediaAuthentication';
@@ -181,7 +183,10 @@ export const UrlPreviewHolder = as<'div'>(({ children, ...props }, ref) => {
             <>
               <div className={css.UrlPreviewHolderGradient({ position: 'Left' })} />
               <IconButton
-                className={css.UrlPreviewHolderBtn({ position: 'Left' })}
+                className={classNames(
+                  css.UrlPreviewHolderBtn({ position: 'Left' }),
+                  PhoneHitInPlace
+                )}
                 variant="Secondary"
                 radii="Pill"
                 size="300"
@@ -199,7 +204,10 @@ export const UrlPreviewHolder = as<'div'>(({ children, ...props }, ref) => {
               <>
                 <div className={css.UrlPreviewHolderGradient({ position: 'Right' })} />
                 <IconButton
-                  className={css.UrlPreviewHolderBtn({ position: 'Right' })}
+                  className={classNames(
+                    css.UrlPreviewHolderBtn({ position: 'Right' }),
+                    PhoneHitInPlace
+                  )}
                   variant="Primary"
                   radii="Pill"
                   size="300"

src/app/phoneHit.coverage.test.ts

@@ -0,0 +1,58 @@
+import { readdirSync, readFileSync } from 'fs';
+import { describe, expect, it } from 'vitest';
+import { classLists, emitted, expectPhoneHitClasses, phoneHitClasses } from './phoneHitTestKit';
+
+// Every IconButton under src carries a PhoneHit class, or is named here with the reason it has none. The per-group
+// suites pin which variant a listed file's buttons carry; this one sees files no group lists, so a new bare
+// IconButton fails here instead of shipping with a sub-44px phone hit area.
+const EXEMPT: Record<string, { indices: number[]; why: string }> = {
+  'features/estate/EstateNavigator.tsx': {
+    indices: [0],
+    why: 'Find a room sits in a css.Targets row that already gives it a 44 px phone target',
+  },
+  'features/lobby/LobbyHeader.tsx': {
+    indices: [1],
+    why: 'Members button, hidden on mobile',
+  },
+};
+
+const tsxFiles = (dir: string): string[] =>
+  readdirSync(`${process.cwd()}/src/app/${dir}`, { withFileTypes: true }).flatMap((e) => {
+    if (e.isDirectory()) return tsxFiles(`${dir}${e.name}/`);
+    return e.name.endsWith('.tsx') ? [`${dir}${e.name}`] : [];
+  });
+
+describe('phone hit areas: every IconButton is covered or named', () => {
+  it('lists exactly the IconButtons without a PhoneHit class', () => {
+    const bare: Record<string, number[]> = {};
+    tsxFiles('').forEach((file) => {
+      const src = readFileSync(`${process.cwd()}/src/app/${file}`, 'utf8');
+      if (!src.includes('<IconButton')) return;
+      const missing = phoneHitClasses(src).flatMap((c, i) => (c === null ? [i] : []));
+      if (missing.length > 0) bare[file] = missing;
+    });
+    expect(bare).toEqual(
+      Object.fromEntries(Object.entries(EXEMPT).map(([f, e]) => [f, e.indices]))
+    );
+  });
+});
+
+// UrlPreviewCard's arrows are absolutely positioned by their own class; PhoneHit would set position: relative on them
+// and win or lose by stylesheet order, so they carry the variant that adds only the hit area.
+describe('phone hit area of a button positioned by its own class', () => {
+  it('the arrows carry PhoneHitInPlace', () => {
+    expectPhoneHitClasses('components/url-preview/UrlPreviewCard.tsx', [
+      'PhoneHitInPlace',
+      'PhoneHitInPlace',
+    ]);
+  });
+
+  it('PhoneHitInPlace draws the ::after and sets no position of its own', async () => {
+    const lists = await classLists('styles/PhoneHit.css.ts');
+    expect(lists.PhoneHitInPlace).toHaveLength(1);
+    const css = await emitted('styles/PhoneHit.css.ts');
+    const rules = [...css.matchAll(/^([^{}\n]*PhoneHitInPlace__[^{}\n]*) \{$/gm)].map((m) => m[1]);
+    expect(rules.length).toBeGreaterThan(0);
+    rules.forEach((sel) => expect(sel).toMatch(/::after$/));
+  });
+});

src/app/phoneHitTestKit.ts

@@ -168,7 +168,7 @@ export const phoneHitOf = (tag: string) => {
 
 /** The PhoneHit* class of each IconButton of a source, in source order; null when it has none or the name is not
  * the one imported from PhoneHit.css. */
-const phoneHitClasses = (src: string) => {
+export const phoneHitClasses = (src: string) => {
   const sf = tsx(src);
   const imported = importedHits(sf);
   return elements(sf).map((el) => hitOf(el, imported));

src/app/phoneTapTargets.test.ts

@@ -102,13 +102,16 @@ describe('phone icon-button hit areas', () => {
     expect(css).toMatch(/::after \{[^}]*transform: translate\(-50%, -50%\);/);
     // the box anchors to its own button, receives the tap, and sits above nothing it should not
     expect(css).toMatch(/@media [^{]+ \{\n\s*\.PhoneHit_PhoneHit__\w+ \{\n\s*position: relative;/);
-    const afterDecls = [...css.matchAll(/::after \{\n([^}]*)\}/g)].flatMap((m) =>
+    // PhoneHit and PhoneHitInPlace each draw it; the Spread and Room classes compose PhoneHit
+    const afters = [...css.matchAll(/::after \{\n([^}]*)\}/g)];
+    expect(afters).toHaveLength(2);
+    const afterDecls = afters.flatMap((m) =>
       m[1]
         .split('\n')
         .map((l) => l.trim().split(':')[0])
         .filter(Boolean)
     );
-    expect([...afterDecls].sort()).toEqual([
+    expect([...new Set(afterDecls)].sort()).toEqual([
       'content',
       'height',
       'left',
@@ -117,8 +120,9 @@ describe('phone icon-button hit areas', () => {
       'transform',
       'width',
     ]);
-    // nothing on a phone moves a button off its own anchor: only relative (the button) and absolute (the box)
+    // nothing on a phone moves a button off its own anchor: only relative (the button) and absolute (the boxes)
     expect([...css.matchAll(/position: (\w+);/g)].map((m) => m[1]).sort()).toEqual([
+      'absolute',
       'absolute',
       'relative',
     ]);

src/app/styles/PhoneHit.css.ts

@@ -29,6 +29,10 @@ export const PhoneHit = style({
   selectors: { '&::after': hitArea },
 });
 
+/** For a button that is already positioned (absolute or fixed) by its own class: PhoneHit's `position: relative`
+ * would fight that class by stylesheet order, and the positioned button already contains its ::after. */
+export const PhoneHitInPlace = style({ selectors: { '&::after': hitArea } });
+
 const spreads = new Map<number, string>();
 
 /** An icon button in a row, `visual` px wide: the row opens up so its 44 px hit area meets its neighbours'. */