fix(#1487): BYOP modal — bounded header, no body occlusion (#1493)

## Fixes #1487 Reporter (@EldoonNemar): "The dialog text can't be seen due to the title bar being massive." ### Root cause `.byop-header` swelled to ~73px on mobile because: 1. `position: sticky` + `margin: -24px -24px 12px` assumed `.modal` desktop padding (24px) — but `.modal` switches to 16px padding at mobile, so the sibling-margin pushed the description paragraph UP into the sticky-pinned header band, occluding it. 2. `.btn-icon` close button floors at 48×48 (touch target) → forced header height ≥48px+padding. 3. H3 inherited a default emoji line-height that added more height on platforms with tall emoji ascent metrics. ### Fix (`public/style.css`) - Drop full-bleed negative-margin gymnastics — header uses normal in-flow padding (`4px 0`); `.modal` padding handles inset. - `max-height: 48px` on header so emoji ascent / btn-icon floor can't blow it past safe range. - Bound H3 explicitly (`font-size: 1rem; line-height: 1.3`). - Override `.byop-x` to compact 32px visual size; preserve ≥44px effective tap target via invisible `::before` pad (a11y safe). ### Verification Hot-swapped onto staging, CDP-measured both viewports: | viewport | hdrH | descTop ≥ hdrBottom | result | |---|---|---|---| | 390×844 mobile | 41px (was 73) | 341 ≥ 329 ✅ | clean | | 1280×800 desktop | 41px | 318 ≥ 306 ✅ | clean | ### TDD - **Red commit**: bb1a9f48 — `test-issue-1487-byop-modal-layout-e2e.js` asserts header ≤56px AND description top ≥ header bottom on both viewports. Pre-fix: header=73px ⇒ FAIL. - **Green commit**: 72a69b3e — CSS fix; assertions all pass against hot-swapped staging. - E2E added: `test-issue-1487-byop-modal-layout-e2e.js`; wired into `.github/workflows/deploy.yml` e2e job. ### Screenshots Before (mobile): description "Paste raw hex bytes..." clipped by oversized header. After: header 41px, description fully visible above textarea. --------- Co-authored-by: corescope-bot <bot@corescope.local>
2026-06-04 02:21:22 +00:00 · 2026-05-29 07:29:57 -07:00
parent 022f3d8f0d
commit c841dbccdd
3 changed files with 159 additions and 4 deletions
@@ -341,6 +341,7 @@ jobs:
          CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-channels-add-modal-e2e.js 2>&1 | tee -a e2e-output.txt
          CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-channels-share-color-e2e.js 2>&1 | tee -a e2e-output.txt
          CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-channels-ws-batch-e2e.js 2>&1 | tee -a e2e-output.txt
+          CHROMIUM_REQUIRE=1 BASE_URL=http://localhost:13581 node test-issue-1487-byop-modal-layout-e2e.js 2>&1 | tee -a e2e-output.txt

      - name: Collect frontend coverage (parallel)
        if: success() && github.event_name == 'push'
@@ -1333,18 +1333,45 @@ body.scroll-locked { overflow: hidden; }
 .byop-modal .byop-header {
  position: sticky; top: 0; z-index: 2;
  background: var(--card-bg);
-  margin: -24px -24px 12px;
-  padding: 12px 24px;
+  /* #1487: was `margin: -24px -24px 12px; padding: 12px 24px;` — assumed
+     .modal desktop padding of 24px. On mobile .modal switches to 16px
+     padding, so the negative margin overshot, content-flow pushed the
+     next sibling UP under the (sticky-pinned) header, and the
+     description paragraph was visually occluded by ~12px. Drop the
+     full-bleed negative-margin gymnastics — header now uses normal
+     padding (modal padding handles the inset) and bounded sizing. */
+  margin: 0 0 12px;
+  padding: 4px 0;
  border-bottom: 1px solid var(--border);
  display: flex; align-items: center; justify-content: space-between; gap: 8px;
+  /* Hard cap so emoji ascent / btn-icon 48px floor can't blow the header
+     past the safe range (was 73px pre-fix). */
+  max-height: 48px;
+  box-sizing: border-box;
+}
+.byop-modal .byop-header h3 {
+  /* #1487: explicit font + line-height so emoji-bearing title doesn't
+     blow up the header on platforms where the default emoji font has
+     huge ascent metrics (observed contributor to 73px header). */
+  margin: 0; font-size: 1rem; line-height: 1.3;
 }
 /* MINOR-7: dropped position: sticky on .byop-x — it sits inside the
   already-sticky .byop-header, so the sticky on the child was a no-op.
-   We only need visual styling here. */
+   We only need visual styling here.
+   #1487: override .btn-icon 48px floor — inside the header we need a
+   compact button so the header stays bounded. The :before tap-target
+   pad below preserves accessibility (effective hit area ~ 44x44). */
 .byop-modal .byop-x {
  background: var(--card-bg); border: 1px solid var(--border);
-  border-radius: 6px; padding: 4px 10px; cursor: pointer; font-size: 16px;
+  border-radius: 6px; padding: 2px 8px; cursor: pointer; font-size: 14px;
  line-height: 1; color: var(--text);
+  min-height: 0; min-width: 0; height: 32px;
+  position: relative;
+}
+/* #1487: invisible tap-pad to keep ≥44px effective hit area while the
+   visual button stays compact inside the bounded header. */
+.byop-modal .byop-x::before {
+  content: ''; position: absolute; inset: -8px; border-radius: 8px;
 }
 .byop-modal .byop-x:hover { background: var(--row-hover, rgba(0,0,0,0.05)); }

@@ -0,0 +1,127 @@
+/**
+ * E2E (#1487): BYOP modal must render with a usable layout — the title bar
+ * must NOT consume most of the dialog height and the body controls
+ * (description, textarea, Decode button) must be visible and not occluded
+ * by the sticky header.
+ *
+ * Reporter: @EldoonNemar — "The dialog text can't be seen due to the title
+ * bar being massive."
+ *
+ * Repro:
+ *   1. Open /#/packets on mobile (390x844).
+ *   2. Click the 📦 BYOP toolbar button.
+ *   3. Observe the modal: the .byop-header swells (~73px tall) and the
+ *      next-sibling description paragraph (`.text-muted`) starts INSIDE
+ *      the sticky-header band, getting visually clipped/occluded.
+ *
+ * Root cause: `.byop-header` uses `position: sticky` + a negative
+ * `margin: -24px -24px 12px` that assumes desktop `.modal` padding of
+ * 24px — but `.modal` switches to 16px padding on mobile. The close
+ * button's box (border + padding) further inflates the header. The
+ * description paragraph then begins at top≈85 inside a header that
+ * spans 24–97, hiding the text.
+ *
+ * Fix expectation:
+ *   - Header height is bounded (<= 56px is a reasonable target).
+ *   - The description paragraph's top edge is BELOW the sticky-header
+ *     bottom edge — i.e. no visual occlusion.
+ *   - The textarea and Decode button are fully within the modal client rect.
+ *
+ * Usage: BASE_URL=http://localhost:13581 node test-issue-1487-byop-modal-layout-e2e.js
+ */
+'use strict';
+const { chromium } = require('playwright');
+
+const BASE = process.env.BASE_URL || 'http://localhost:13581';
+
+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 probeModal(page) {
+  return page.evaluate(() => {
+    const m = document.querySelector('.byop-modal');
+    if (!m) return { err: 'no modal' };
+    const hdr = m.querySelector('.byop-header');
+    const desc = m.querySelector('.text-muted');
+    const ta = m.querySelector('.byop-input');
+    const btn = m.querySelector('#byopDecode');
+    const mr = m.getBoundingClientRect();
+    const hr = hdr.getBoundingClientRect();
+    const dr = desc.getBoundingClientRect();
+    const tr = ta.getBoundingClientRect();
+    const br = btn.getBoundingClientRect();
+    return {
+      modalH: Math.round(mr.height),
+      hdrH: Math.round(hr.height),
+      hdrBottom: Math.round(hr.bottom),
+      descTop: Math.round(dr.top),
+      taBottom: Math.round(tr.bottom),
+      btnBottom: Math.round(br.bottom),
+      modalBottom: Math.round(mr.bottom),
+    };
+  });
+}
+
+async function openBYOP(page) {
+  await page.waitForSelector('[data-action="pkt-byop"]', { timeout: 8000 });
+  await page.evaluate(() => {
+    document.querySelectorAll('.byop-overlay').forEach(o => o.remove());
+    document.querySelector('[data-action="pkt-byop"]').click();
+  });
+  await page.waitForSelector('.byop-modal', { timeout: 5000 });
+  await page.waitForTimeout(200);
+}
+
+async function runViewport(browser, label, viewport) {
+  const ctx = await browser.newContext({ viewport });
+  const page = await ctx.newPage();
+  page.setDefaultTimeout(10000);
+  page.on('pageerror', (e) => console.error('[pageerror]', e.message));
+
+  console.log(`\n--- viewport ${label} (${viewport.width}x${viewport.height}) ---`);
+
+  await step('navigate to /packets and open BYOP', async () => {
+    await page.goto(BASE + '/#/packets', { waitUntil: 'domcontentloaded' });
+    await openBYOP(page);
+  });
+
+  await step('header height is bounded (<= 56px)', async () => {
+    const p = await probeModal(page);
+    assert(p.hdrH <= 56, `header height ${p.hdrH}px > 56px cap (modal=${p.modalH}px)`);
+  });
+
+  await step('description paragraph is NOT occluded by sticky header', async () => {
+    const p = await probeModal(page);
+    assert(p.descTop >= p.hdrBottom,
+      `description top (${p.descTop}) starts INSIDE sticky header band (header bottom=${p.hdrBottom})`);
+  });
+
+  await step('textarea and Decode button do not overflow modal client rect', async () => {
+    const p = await probeModal(page);
+    assert(p.taBottom <= p.modalBottom + 1, `textarea bottom (${p.taBottom}) overflows modal (${p.modalBottom})`);
+    assert(p.btnBottom <= p.modalBottom + 1, `Decode button bottom (${p.btnBottom}) overflows modal`);
+  });
+
+  await ctx.close();
+}
+
+(async () => {
+  const browser = await chromium.launch({
+    headless: true,
+    executablePath: process.env.CHROMIUM_PATH || undefined,
+    args: ['--no-sandbox', '--disable-gpu', '--disable-dev-shm-usage'],
+  });
+
+  console.log(`\n=== #1487 BYOP modal layout E2E against ${BASE} ===`);
+
+  await runViewport(browser, 'mobile', { width: 390, height: 844 });
+  await runViewport(browser, 'desktop', { width: 1280, height: 800 });
+
+  await browser.close();
+  console.log(`\n${passed} passed, ${failed} failed`);
+  process.exit(failed === 0 ? 0 : 1);
+})().catch(e => { console.error(e); process.exit(1); });