diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 108d2b970..19f1ad900 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -112,6 +112,7 @@ jobs: node test-issue-1518-home-url.js node test-channel-decrypt-insecure-context.js node test-live-region-filter.js + node test-issue-2052-touch-target-css.js node test-issue-1136-observer-iata-map.js node test-channel-qr.js node test-channel-qr-wiring.js @@ -478,6 +479,7 @@ jobs: CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-issue-1657-analytics-channels-group-sprites-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1224-channels-mobile-ux-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1367-channels-chat-app-e2e.js 2>&1 | tee -a e2e-output.txt + CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-issue-2052-touch-target-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1236-map-mobile-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1329-map-controls-accordion-e2e.js 2>&1 | tee -a e2e-output.txt BASE_URL=http://localhost:13581 node test-issue-1273-qr-overlay-height-e2e.js 2>&1 | tee -a e2e-output.txt diff --git a/public/style.css b/public/style.css index 0b6067fb4..7cf997e50 100644 --- a/public/style.css +++ b/public/style.css @@ -518,8 +518,10 @@ a:focus-visible, button:focus-visible, input:focus-visible, select:focus-visible } /* === Touch Targets === */ -/* WCAG 2.5.5 / Apple HIG / Material: 48x48 CSS px minimum touch target for - all interactive controls. Targets are achieved with min-height/min-width +/* WCAG 2.5.5 and Apple HIG use a 44x44 CSS px target; Material uses 48x48. + Most controls in this section use the 48x48 house preference. Compact + .nav-btn and .ch-icon-btn controls retain 44x44 in their component rules. + Targets are achieved with min-height/min-width plus inline-flex centering so existing visual styling (font-size, padding, icon size) is preserved on desktop while the *hit area* grows for touch. Issue #1060. @@ -558,13 +560,6 @@ a:focus-visible, button:focus-visible, input:focus-visible, select:focus-visible touch-action: manipulation; } -.nav-btn { - min-height: 48px; - min-width: 48px; - touch-action: manipulation; -} - -.ch-icon-btn, .ch-remove-btn, .ch-share-btn { min-height: 48px; @@ -891,6 +886,7 @@ img.brand-logo { background: none; border: 1px solid var(--border); color: var(--nav-text-muted); padding: 6px 12px; border-radius: 6px; cursor: pointer; font-size: 14px; transition: all .15s; min-width: 44px; min-height: 44px; display: inline-flex; align-items: center; justify-content: center; + touch-action: manipulation; } .nav-btn:hover { background: var(--nav-bg2); color: var(--nav-text); } @@ -1870,6 +1866,7 @@ button.ch-item.selected { background: var(--selected-bg); } background: none; border: none; color: var(--text-muted); cursor: pointer; padding: 4px; margin-left: 2px; min-width: 44px; min-height: 44px; box-sizing: border-box; + touch-action: manipulation; opacity: 0.55; transition: opacity 0.15s, color 0.15s; line-height: 1; user-select: none; } @@ -2464,19 +2461,16 @@ button.ch-item:hover .ch-icon-btn { opacity: 1; } .ch-header-region .region-pill { padding: 2px 6px; font-size: 11px; } .ch-header-region .region-dropdown-trigger { padding: 3px 6px; font-size: 11px; } - /* Channel list row: name must dominate. The inline share/remove icons - were eating >88px because of their 44x44 touch target — push them - onto a per-row reveal pattern: show a single ⋮ trigger that toggles - the action set. Until then, shrink them to icon-only and let the - name grow via flex:1. */ + /* Channel list row: name must dominate. Below 768px channels.js renders + the flat .ch-row list (#1367), which has no inline share/remove + actions, so there is no size override for .ch-icon-btn here: wherever + those actions render they keep the 44x44 target from the base rule. */ #chList .ch-item { padding: 10px 12px; gap: 10px; } #chList .ch-item-name { flex: 1 1 auto; min-width: 0; } #chList .ch-item-top { gap: 6px; } - #chList .ch-icon-btn { min-width: 32px; min-height: 32px; padding: 2px 4px; } /* #1648 M5: share-btn now renders an inline from channels.js, * so the legacy `font-size: 0` + `::before { content: '' }` shim is - * obsolete. The icon-button class above already sets the touch target; - * the SVG inherits color via currentColor. */ + * obsolete. The SVG inherits color via currentColor. */ #chList .ch-share-btn .ph-icon { font-size: 13px; } #chList .ch-share-btn { font-size: 0; } #chList .ch-share-btn .ph-icon, #chList .ch-share-btn svg { font-size: 13px; } diff --git a/test-all.sh b/test-all.sh index 0e63d460f..868f0609e 100755 --- a/test-all.sh +++ b/test-all.sh @@ -57,6 +57,7 @@ node test-issue-1883-redirect-history.js node test-issue-1890-og-url.js node test-traces.js node test-live-multibyte-filter.js +node test-issue-2052-touch-target-css.js # #1418 — route-view v2 (Tufte) coverage node test-issue-1418-raw-hex-extraction.js diff --git a/test-issue-2052-touch-target-css.js b/test-issue-2052-touch-target-css.js new file mode 100644 index 000000000..0f2a677ea --- /dev/null +++ b/test-issue-2052-touch-target-css.js @@ -0,0 +1,86 @@ +#!/usr/bin/env node +/* Issue #2052 — static touch-target policy for the compact controls. + * + * Supplement only: test-issue-2052-touch-target-e2e.js measures the rendered + * boxes in a browser and is the real guarantee. + * + * This test reads the shipped stylesheet and, for every rule anywhere in it + * (including inside @media blocks), looks at each selector whose last compound + * targets .nav-btn, .ch-icon-btn, .ch-share-btn or .ch-remove-btn. More + * specific selectors such as `#chList .ch-icon-btn` count too; an earlier + * version matched only the exact selector text and missed a 32px override. + * Every min-width/min-height declared for these controls must be at least + * 44px. Pseudo-class rules (:hover, :active, :focus, ...) are skipped because + * they do not set target dimensions. The base .nav-btn and .ch-icon-btn rules + * must also keep touch-action: manipulation. + */ +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); + +const css = fs.readFileSync(path.join(__dirname, 'public', 'style.css'), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, ''); + +const CONTROLS = ['.nav-btn', '.ch-icon-btn', '.ch-share-btn', '.ch-remove-btn']; + +// The innermost { } blocks are the style rules, also inside @media. +function rules() { + const out = []; + const rulePattern = /([^{}]+)\{([^{}]*)\}/g; + let match; + while ((match = rulePattern.exec(css)) !== null) { + out.push({ selectors: match[1].split(',').map((s) => s.trim()), body: match[2] }); + } + return out; +} + +// The compact control a selector targets, or null. Only the last compound +// counts (`.ch-icon-btn .ph-icon` targets the icon, not the button). +function targetedControl(selector) { + const last = selector.split(/[\s>+~]+/).filter(Boolean).pop() || ''; + if (/:/.test(last)) return null; + return CONTROLS.find((c) => new RegExp('\\' + c + '(?![\\w-])').test(last)) || null; +} + +function declared(body, property) { + const values = []; + const pattern = new RegExp(`(?:^|;)\\s*${property}\\s*:\\s*([^;]+)`, 'g'); + let m; + while ((m = pattern.exec(body)) !== null) values.push(m[1].trim()); + return values; +} + +const violations = []; +const seen = new Set(); +let checked = 0; +for (const rule of rules()) { + for (const selector of rule.selectors) { + const control = targetedControl(selector); + if (!control) continue; + seen.add(control); + for (const property of ['min-width', 'min-height']) { + for (const value of declared(rule.body, property)) { + checked++; + const px = /^(\d+(?:\.\d+)?)px$/.exec(value); + if (!px || Number(px[1]) < 44) violations.push(`${selector} { ${property}: ${value} }`); + } + } + } +} + +assert.deepStrictEqual(CONTROLS.filter((c) => !seen.has(c)), [], 'every compact control must have at least one rule'); +assert(checked > 0, 'no min-width/min-height declarations found for the compact controls'); +assert.deepStrictEqual(violations, [], 'compact controls must not declare a touch target below 44px:\n ' + violations.join('\n ')); +console.log(`PASS ${checked} min-width/min-height declarations for ${CONTROLS.join(', ')} are all >= 44px`); + +for (const base of ['.nav-btn', '.ch-icon-btn']) { + const values = rules() + .filter((r) => r.selectors.includes(base)) + .flatMap((r) => declared(r.body, 'touch-action')); + assert.deepStrictEqual([...new Set(values)], ['manipulation'], `${base} must keep touch-action: manipulation; got ${JSON.stringify(values)}`); + console.log(`PASS ${base}: touch-action manipulation`); +} + +console.log('test-issue-2052-touch-target-css.js: OK'); diff --git a/test-issue-2052-touch-target-e2e.js b/test-issue-2052-touch-target-e2e.js new file mode 100644 index 000000000..15799e51f --- /dev/null +++ b/test-issue-2052-touch-target-e2e.js @@ -0,0 +1,266 @@ +#!/usr/bin/env node +/* Issue #2052 — rendered touch targets for the channel row actions. + * + * test-issue-2052-touch-target-css.js checks the stylesheet text; this test + * measures what the browser actually renders, in the real responsive DOM: + * + * - Desktop 1440x900 (sectioned .ch-item list): the share and remove actions + * of a user-added channel are at least 44x44 CSS px, visible, not clipped + * by any overflow container, hit at their centre, not overlapping each + * other or the row's other parts, reachable by keyboard with focus-visible, + * keep their ARIA/title contract, and do their job without side effects + * (share opens the modal, remove asks for confirmation, which is dismissed). + * - Mobile 390x844 touch (flat .ch-row list, #1367): the list renders no + * share/remove actions at all, every visible tap target in the channel + * sidebar is at least 44x44, and tapping a row opens the channel. + * - Tablet 768x1024 touch: characterization only. The sidebar is narrower + * than a channel row there, so the actions are clipped. That is a known, + * separate layout issue (also on master); this step logs the measurement + * and does not gate. + * + * The user-added channel comes from a saved key in localStorage, the same + * state a returning user has; no DOM is injected. No sleeps or retries: each + * step waits for the element it needs. + * + * Defaults to localhost:13581 — NEVER point at prod (AGENTS.md). CI sets BASE_URL. + * Run: BASE_URL=http://localhost:13581 node test-issue-2052-touch-target-e2e.js + */ +'use strict'; +const { chromium } = require('playwright'); + +const BASE = process.env.BASE_URL || 'http://localhost:13581'; +const ORIGIN = new URL(BASE).origin; +const CHANNEL = '#tt2052probe'; +const HASH = 'user:' + CHANNEL; +const SHARE = `#chList [data-share-channel="${HASH}"]`; +const REMOVE = `#chList [data-remove-channel="${HASH}"]`; +const MIN = 44; + +let passed = 0, failed = 0; +async function step(name, fn) { + try { await fn(); passed++; console.log(' ✓ ' + name); } + catch (e) { failed++; console.error(' ✗ ' + name + ': ' + e.message); } +} +function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } + +async function openChannels(browser, errors, options) { + const ctx = await browser.newContext(options); + const page = await ctx.newPage(); + page.setDefaultTimeout(15000); + page.on('pageerror', (e) => errors.push('pageerror: ' + e.message)); + page.on('console', (m) => { + if (m.type() !== 'error') return; + // Map tiles and other CDN resources are outside this app. + const url = (m.location() && m.location().url) || ''; + if (url && !url.startsWith(ORIGIN)) return; + errors.push('console.error: ' + m.text()); + }); + await page.addInitScript(([name]) => { + localStorage.setItem('corescope_channel_keys', JSON.stringify({ [name]: '00112233445566778899aabbccddeeff' })); + window.addEventListener('unhandledrejection', (e) => { + console.error('unhandledrejection: ' + ((e.reason && e.reason.message) || e.reason)); + }); + }, [CHANNEL]); + await page.goto(BASE + '/#/channels', { waitUntil: 'domcontentloaded' }); + return { ctx, page }; +} + +// Keyboard focus lands on the control, matches :focus-visible, and the +// existing focus rule (.ch-icon-btn:focus { opacity: 1 }) takes effect. The +// opacity has a 0.15s transition, so wait for its end state, not a time. +// This checks the focus state and the existing opacity cue only; the same +// rule also sets outline: none (unchanged here, as on master), so no focus +// ring is asserted. +async function assertFocused(page, selector, name) { + const st = await page.evaluate((sel) => { + const e = document.querySelector(sel); + return { focused: document.activeElement === e, visible: e.matches(':focus-visible') }; + }, selector); + assert(st.focused, name + ' is not focused'); + assert(st.visible, name + ' focus does not match :focus-visible'); + await page.waitForFunction((sel) => getComputedStyle(document.querySelector(sel)).opacity === '1', selector) + .catch((e) => { throw new Error(name + ' never reached the focused opacity of 1: ' + e.message); }); +} + +// Geometry of one control, measured in the page. +function measure(page, selector) { + return page.evaluate((sel) => { + const el = document.querySelector(sel); + if (!el) return null; + const r = el.getBoundingClientRect(); + const cs = getComputedStyle(el); + // How far the box sticks out of the viewport and of every clipping ancestor. + let clippedBy = []; + const outside = (box, name) => { + const over = Math.max(box.left - r.left, r.right - box.right, box.top - r.top, r.bottom - box.bottom); + if (over > 0.5) clippedBy.push(name + ' by ' + over.toFixed(1) + 'px'); + }; + outside({ left: 0, top: 0, right: innerWidth, bottom: innerHeight }, 'viewport'); + for (let a = el.parentElement; a; a = a.parentElement) { + const c = getComputedStyle(a); + if (c.overflowX !== 'visible' || c.overflowY !== 'visible') outside(a.getBoundingClientRect(), a.id || String(a.className).split(' ')[0] || a.tagName); + } + const hitEl = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2); + return { + w: r.width, h: r.height, left: r.left, right: r.right, top: r.top, bottom: r.bottom, + visible: cs.visibility === 'visible' && cs.display !== 'none' && Number(cs.opacity) > 0 && r.width > 0 && r.height > 0, + clippedBy, + hit: !!hitEl && (hitEl === el || el.contains(hitEl)), + hitOn: hitEl ? (hitEl.id || String(hitEl.className) || hitEl.tagName) : 'nothing', + }; + }, selector); +} + +function overlaps(a, b) { + return !(a.right <= b.left || b.right <= a.left || a.bottom <= b.top || b.bottom <= a.top); +} + +async function main() { + let browser; + try { + browser = await chromium.launch({ + headless: true, + executablePath: process.env.CHROMIUM_PATH || undefined, + args: ['--no-sandbox', '--disable-gpu', '--disable-dev-shm-usage'], + }); + } catch (err) { + if (process.env.CHROMIUM_REQUIRE === '1') { + console.error('test-issue-2052-touch-target-e2e.js: FAIL — Chromium required but unavailable: ' + err.message); + process.exit(1); + } + console.log('test-issue-2052-touch-target-e2e.js: SKIP (Chromium unavailable: ' + err.message.split('\n')[0] + ')'); + process.exit(0); + } + const errors = []; + console.log(`\n=== #2052 channel action touch targets E2E against ${BASE} ===`); + + // ── Desktop: the sectioned list with inline actions ────────────────────── + const desk = await openChannels(browser, errors, { viewport: { width: 1440, height: 900 } }); + const dp = desk.page; + try { + await dp.waitForSelector(REMOVE, { state: 'visible' }); + + await step('desktop 1440x900: share and remove are at least 44x44, visible, unclipped and hit at their centre', async () => { + for (const [name, sel] of [['share', SHARE], ['remove', REMOVE]]) { + const m = await measure(dp, sel); + assert(m, name + ' action not rendered'); + assert(m.w >= MIN && m.h >= MIN, `${name} renders ${m.w.toFixed(1)}x${m.h.toFixed(1)}, below ${MIN}x${MIN}`); + assert(m.visible, name + ' is not visible'); + assert(m.clippedBy.length === 0, name + ' is clipped by ' + m.clippedBy.join(', ')); + assert(m.hit, `${name} centre hits "${m.hitOn}", not the control`); + } + }); + + await step('desktop: actions do not overlap each other or the rest of the row, and the page has no horizontal overflow', async () => { + const share = await measure(dp, SHARE), remove = await measure(dp, REMOVE); + assert(!overlaps(share, remove), 'share and remove overlap'); + const parts = await dp.evaluate((sel) => { + const row = document.querySelector(sel).closest('.ch-item'); + return [...row.querySelectorAll('.ch-badge, .ch-item-name, .ch-user-badge, .ch-unread-badge, .ch-color-dot, .ch-color-clear, .ch-item-time, .ch-item-preview')] + .map((e) => { const r = e.getBoundingClientRect(); return { name: String(e.className).split(' ')[0], left: r.left, right: r.right, top: r.top, bottom: r.bottom }; }); + }, SHARE); + assert(parts.length >= 4, 'row parts not found'); + for (const p of parts) { + assert(!overlaps(share, p), 'share overlaps ' + p.name); + assert(!overlaps(remove, p), 'remove overlaps ' + p.name); + } + const overflow = await dp.evaluate(() => document.documentElement.scrollWidth - document.documentElement.clientWidth); + assert(overflow <= 0, 'page overflows horizontally by ' + overflow + 'px'); + }); + + await step('desktop: ARIA and title contract is unchanged', async () => { + const a = await dp.evaluate(([s, r]) => [s, r].map((sel) => { + const e = document.querySelector(sel); + return { role: e.getAttribute('role'), tabindex: e.getAttribute('tabindex'), label: e.getAttribute('aria-label'), title: e.getAttribute('title'), popup: e.getAttribute('aria-haspopup') }; + }), [SHARE, REMOVE]); + assert(a[0].role === 'button' && a[0].tabindex === '0' && a[0].label === 'Share ' + CHANNEL && a[0].title === 'Share channel key (QR + URL)' && a[0].popup === 'dialog', + 'share ARIA/title changed: ' + JSON.stringify(a[0])); + assert(a[1].role === 'button' && a[1].tabindex === '0' && a[1].label === 'Remove ' + CHANNEL && a[1].title === 'Remove channel and clear saved key', + 'remove ARIA/title changed: ' + JSON.stringify(a[1])); + }); + + await step('desktop: Tab reaches share then remove with focus-visible, and Enter runs each action without side effects', async () => { + await dp.focus(`#chList .ch-item[data-hash="${HASH}"]`); + await dp.keyboard.press('Tab'); + await assertFocused(dp, SHARE, 'share (Tab from the row)'); + await dp.keyboard.press('Enter'); + await dp.waitForSelector('#chShareModal:not(.hidden)'); + await dp.keyboard.press('Escape'); + await dp.waitForFunction(() => document.getElementById('chShareModal').classList.contains('hidden')); + + await dp.focus(SHARE); // the closed modal does not have to hand focus back + await dp.keyboard.press('Tab'); + await assertFocused(dp, REMOVE, 'remove (Tab from share)'); + // waitForEvent has a timeout, so a missing confirmation fails the step + // instead of hanging the run. + const dialog = dp.waitForEvent('dialog', { timeout: 10000 }) + .then(async (d) => { const text = d.message(); await d.dismiss(); return text; }); + await dp.keyboard.press('Enter'); + const text = await dialog; + assert(/Remove channel/.test(text), 'remove did not ask for confirmation: ' + text); + const kept = await dp.evaluate(([sel, name]) => !!document.querySelector(sel) && + !!JSON.parse(localStorage.getItem('corescope_channel_keys') || '{}')[name], [REMOVE, CHANNEL]); + assert(kept, 'dismissing the confirmation still removed the channel'); + }); + } finally { + await desk.ctx.close(); + } + + // ── Mobile: the flat chat-app list, no inline actions ──────────────────── + const mob = await openChannels(browser, errors, { viewport: { width: 390, height: 844 }, hasTouch: true, isMobile: true }); + const mp = mob.page; + try { + // Wait for the channel's row, whatever layout rendered it; the step below + // asserts which layout it is. + await mp.waitForSelector(`#chList [data-hash="${HASH}"]`, { state: 'visible' }); + + await step('mobile 390x844: the list renders no share/remove actions, and every visible tap target is at least 44x44', async () => { + const r = await mp.evaluate((hash) => ({ + mobileRow: !!document.querySelector(`#chList .ch-row[data-hash="${hash}"]`), + actions: document.querySelectorAll('#chList .ch-icon-btn, #chList [data-share-channel], #chList [data-remove-channel]').length, + desktopRows: document.querySelectorAll('#chList .ch-item').length, + small: [...document.querySelectorAll('.ch-sidebar button, .ch-sidebar a[href], .ch-sidebar [role="button"], .ch-sidebar [tabindex="0"]')] + .filter((e) => { const b = e.getBoundingClientRect(); return b.width > 0 && b.height > 0 && getComputedStyle(e).visibility === 'visible'; }) + .map((e) => { const b = e.getBoundingClientRect(); return { name: e.id || String(e.className).split(' ')[0] || e.tagName, w: +b.width.toFixed(1), h: +b.height.toFixed(1) }; }) + .filter((t) => t.w < 44 || t.h < 44), + overflow: document.documentElement.scrollWidth - document.documentElement.clientWidth, + }), HASH); + assert(r.mobileRow, 'the channel is not rendered as a mobile .ch-row'); + assert(r.actions === 0 && r.desktopRows === 0, `mobile list renders desktop actions (actions=${r.actions}, .ch-item rows=${r.desktopRows})`); + assert(r.small.length === 0, 'mobile tap targets below 44x44: ' + JSON.stringify(r.small)); + assert(r.overflow <= 0, 'mobile page overflows horizontally by ' + r.overflow + 'px'); + }); + + await step('mobile: tapping the channel row opens the channel', async () => { + await mp.tap(`#chList [data-hash="${HASH}"]`); + await mp.waitForFunction((h) => location.hash.includes('/channels/') && + (location.hash.includes(encodeURIComponent(h)) || location.hash.includes(h)), HASH); + }); + } finally { + await mob.ctx.close(); + } + + // ── Tablet: characterization of the known clipping (not gating) ────────── + const tab = await openChannels(browser, errors, { viewport: { width: 768, height: 1024 }, hasTouch: true }); + try { + await tab.page.waitForSelector(REMOVE, { state: 'attached' }); + await step('tablet 768x1024: characterization only (known, separate layout issue)', async () => { + const share = await measure(tab.page, SHARE), remove = await measure(tab.page, REMOVE); + assert(share && remove, 'tablet actions not rendered'); + const fmt = (n, m) => `${n} ${m.w.toFixed(0)}x${m.h.toFixed(0)} hit=${m.hit ? 'yes' : 'no (' + m.hitOn + ')'} clipped=${m.clippedBy.join('; ') || 'no'}`; + console.log(' KNOWN LIMITATION (tracked separately, not fixed here): ' + fmt('share', share) + ' | ' + fmt('remove', remove)); + }); + } finally { + await tab.ctx.close(); + } + + await step('no page errors, console errors or unhandled rejections', async () => { + assert(errors.length === 0, errors.length + ' error(s):\n ' + errors.join('\n ')); + }); + + await browser.close(); + console.log(`\ntest-issue-2052-touch-target-e2e.js: ${passed} passed, ${failed} failed`); + process.exit(failed > 0 ? 1 : 0); +} + +main().catch((e) => { console.error(e); process.exit(1); });