diff --git a/.claude/skills/critique/scripts/detect-antipatterns-browser.js b/.claude/skills/critique/scripts/detect-antipatterns-browser.js index b5156f7cf..ced838c68 100644 --- a/.claude/skills/critique/scripts/detect-antipatterns-browser.js +++ b/.claude/skills/critique/scripts/detect-antipatterns-browser.js @@ -1262,6 +1262,27 @@ if (IS_BROWSER) { }; window.addEventListener('resize', onResize); + // Track target element visibility via IntersectionObserver. + // Uses a huge rootMargin so all *rendered* elements count as intersecting, + // while display:none / closed
/ hidden modals etc. do not. + // This is event-driven -- no polling needed. + const visibilityObserver = new IntersectionObserver((entries) => { + for (const entry of entries) { + const overlay = entry.target._impeccableOverlay; + if (!overlay) continue; + if (entry.isIntersecting) { + overlay.style.display = ''; + const rect = entry.target.getBoundingClientRect(); + overlay.style.top = `${rect.top + scrollY - 2}px`; + overlay.style.left = `${rect.left + scrollX - 2}px`; + overlay.style.width = `${rect.width + 4}px`; + overlay.style.height = `${rect.height + 4}px`; + } else { + overlay.style.display = 'none'; + } + } + }, { rootMargin: '99999px' }); + const highlight = function(el, findings) { const rect = el.getBoundingClientRect(); const outline = document.createElement('div'); @@ -1299,6 +1320,11 @@ if (IS_BROWSER) { }); outline.appendChild(tooltip); + // Start hidden; the IntersectionObserver will show it once the target is rendered + outline.style.display = 'none'; + el._impeccableOverlay = outline; + visibilityObserver.observe(el); + document.body.appendChild(outline); overlays.push(outline); }; @@ -1356,6 +1382,7 @@ if (IS_BROWSER) { const scan = function() { for (const o of overlays) o.remove(); overlays.length = 0; + visibilityObserver.disconnect(); const allFindings = []; for (const el of document.querySelectorAll('*')) { diff --git a/source/skills/critique/scripts/detect-antipatterns.mjs b/source/skills/critique/scripts/detect-antipatterns.mjs index 6ed207656..0091bccae 100644 --- a/source/skills/critique/scripts/detect-antipatterns.mjs +++ b/source/skills/critique/scripts/detect-antipatterns.mjs @@ -1260,6 +1260,27 @@ if (IS_BROWSER) { }; window.addEventListener('resize', onResize); + // Track target element visibility via IntersectionObserver. + // Uses a huge rootMargin so all *rendered* elements count as intersecting, + // while display:none / closed
/ hidden modals etc. do not. + // This is event-driven -- no polling needed. + const visibilityObserver = new IntersectionObserver((entries) => { + for (const entry of entries) { + const overlay = entry.target._impeccableOverlay; + if (!overlay) continue; + if (entry.isIntersecting) { + overlay.style.display = ''; + const rect = entry.target.getBoundingClientRect(); + overlay.style.top = `${rect.top + scrollY - 2}px`; + overlay.style.left = `${rect.left + scrollX - 2}px`; + overlay.style.width = `${rect.width + 4}px`; + overlay.style.height = `${rect.height + 4}px`; + } else { + overlay.style.display = 'none'; + } + } + }, { rootMargin: '99999px' }); + const highlight = function(el, findings) { const rect = el.getBoundingClientRect(); const outline = document.createElement('div'); @@ -1297,6 +1318,11 @@ if (IS_BROWSER) { }); outline.appendChild(tooltip); + // Start hidden; the IntersectionObserver will show it once the target is rendered + outline.style.display = 'none'; + el._impeccableOverlay = outline; + visibilityObserver.observe(el); + document.body.appendChild(outline); overlays.push(outline); }; @@ -1354,6 +1380,7 @@ if (IS_BROWSER) { const scan = function() { for (const o of overlays) o.remove(); overlays.length = 0; + visibilityObserver.disconnect(); const allFindings = []; for (const el of document.querySelectorAll('*')) { diff --git a/tests/detect-antipatterns-browser.test.js b/tests/detect-antipatterns-browser.test.js index ac55aa7c8..19da39628 100644 --- a/tests/detect-antipatterns-browser.test.js +++ b/tests/detect-antipatterns-browser.test.js @@ -149,3 +149,118 @@ describeIf('browser script parity with CLI', () => { // Browser script doesn't have isFullPage check, so we just verify borders work }, 15000); }); + +describeIf('overlay positioning accuracy', () => { + let browser, page; + const TOLERANCE = 6; // px tolerance for position comparison (2px border offset + rounding) + + beforeAll(async () => { + // Reuse same server setup — these tests run in the same suite + serverProcess = spawn('node', ['-e', ` + const http = require('http'); + const fs = require('fs'); + const path = require('path'); + const types = { '.html': 'text/html', '.css': 'text/css', '.js': 'application/javascript' }; + http.createServer((req, res) => { + let filePath; + if (req.url.startsWith('/fixtures/')) { + filePath = path.join(${JSON.stringify(path.join(import.meta.dir))}, req.url); + } else if (req.url.startsWith('/js/')) { + const basename = req.url.split('/').pop(); + filePath = path.join(${JSON.stringify(path.join(import.meta.dir, '..', 'public'))}, req.url); + if (!fs.existsSync(filePath)) { + filePath = path.join(${JSON.stringify(path.join(import.meta.dir, '..', '.claude', 'skills', 'critique', 'scripts'))}, basename); + } + } else { + res.writeHead(404); res.end(); return; + } + try { + const content = fs.readFileSync(filePath); + const ext = path.extname(filePath); + res.writeHead(200, { 'Content-Type': types[ext] || 'text/plain' }); + res.end(content); + } catch { res.writeHead(404); res.end(); } + }).listen(${PORT + 1}); + `], { stdio: 'ignore' }); + + await new Promise(r => setTimeout(r, 500)); + + browser = await puppeteer.default.launch({ headless: true }); + page = await browser.newPage(); + await page.setViewport({ width: 1280, height: 800 }); + await page.goto(`http://localhost:${PORT + 1}/fixtures/antipatterns/overlay-positioning.html`, { + waitUntil: 'networkidle0', + timeout: 10000, + }); + await new Promise(r => setTimeout(r, 300)); + await page.evaluate(() => { + if (window.impeccableScan) window.impeccableScan(); + }); + await new Promise(r => setTimeout(r, 200)); + }); + + afterAll(async () => { + if (browser) await browser.close(); + if (serverProcess) serverProcess.kill(); + }); + + async function getOverlayPositions() { + return page.evaluate(() => { + const overlays = document.querySelectorAll('.impeccable-overlay:not(.impeccable-banner)'); + return Array.from(overlays).map(o => { + const t = o._targetEl; + if (!t) return null; + const tRect = t.getBoundingClientRect(); + const oRect = o.getBoundingClientRect(); + const label = o.querySelector('.impeccable-label')?.textContent || ''; + return { + label, + target: { top: tRect.top, left: tRect.left, width: tRect.width, height: tRect.height }, + overlay: { top: oRect.top, left: oRect.left, width: oRect.width, height: oRect.height }, + overlayHidden: o.style.display === 'none', + targetVisible: tRect.width > 0 && tRect.height > 0, + inClosedDetails: !!t.closest('details:not([open])'), + }; + }).filter(Boolean); + }); + } + + test('visible overlays are positioned within tolerance of their targets', async () => { + const positions = await getOverlayPositions(); + const visible = positions.filter(p => !p.overlayHidden); + expect(visible.length).toBeGreaterThan(0); + + for (const p of visible) { + const topDiff = Math.abs(p.overlay.top - p.target.top); + const leftDiff = Math.abs(p.overlay.left - p.target.left); + const widthDiff = Math.abs(p.overlay.width - p.target.width); + const heightDiff = Math.abs(p.overlay.height - p.target.height); + + expect(topDiff).toBeLessThanOrEqual(TOLERANCE); + expect(leftDiff).toBeLessThanOrEqual(TOLERANCE); + expect(widthDiff).toBeLessThanOrEqual(TOLERANCE); + expect(heightDiff).toBeLessThanOrEqual(TOLERANCE); + } + }, 20000); + + test('overlays for non-rendered elements are hidden', async () => { + const positions = await getOverlayPositions(); + // Elements inside closed
should have hidden overlays + const closedDetailsOverlays = positions.filter(p => p.inClosedDetails); + expect(closedDetailsOverlays.length).toBeGreaterThan(0); + for (const p of closedDetailsOverlays) { + expect(p.overlayHidden).toBe(true); + } + }, 20000); + + test('overlays inside transform ancestors are accurately positioned', async () => { + const positions = await getOverlayPositions(); + const visible = positions.filter(p => !p.overlayHidden); + for (const p of visible) { + const topDiff = Math.abs(p.overlay.top - p.target.top); + const leftDiff = Math.abs(p.overlay.left - p.target.left); + expect(topDiff).toBeLessThanOrEqual(TOLERANCE); + expect(leftDiff).toBeLessThanOrEqual(TOLERANCE); + } + }, 20000); +}); diff --git a/tests/fixtures/antipatterns/overlay-positioning.html b/tests/fixtures/antipatterns/overlay-positioning.html new file mode 100644 index 000000000..47a6686c7 --- /dev/null +++ b/tests/fixtures/antipatterns/overlay-positioning.html @@ -0,0 +1,524 @@ + + + + + + Overlay Positioning Edge Cases + + + + +

Overlay Positioning Edge Cases

+

Each scenario places a detectable anti-pattern inside a layout context that can cause overlay misalignment.

+ + +

1. Transform Ancestors

+ +
+
1a. translateY(0) container
+
+ This tiny text is inside a transform: translateY(0) container. The overlay should frame this text precisely. +
+
+ +
+
1b. rotate(0deg) container
+
+ Tiny text inside a transform: rotate(0deg) container. +
+
+ +
+
1c. scale(1) container
+
+
Cramped text inside a transform: scale(1) container.
+
+
+ +
+
1d. Nested transforms
+
+
+ Tiny text nested inside two levels of transformed parents. +
+
+
+ +
+
1e. Transform + absolute child
+
+
+ Absolutely positioned tiny text inside transformed parent. +
+
+
+ + +

2. Closed Details

+ +
+
2a. Closed details with anti-pattern inside
+
+ Click to expand (closed by default) + This tiny text is hidden inside a closed details element. No overlay should appear for it. +
Cramped text also hidden inside closed details.
+
+
+ +
+
2b. Open details with anti-pattern inside
+
+ This details is open + This tiny text IS visible because details is open. Overlay should frame it correctly. +
+
+ +
+
2c. Nested details (outer open, inner closed)
+
+ Outer details (open) +

Some visible content.

+
+ Inner details (closed) + Hidden tiny text inside nested closed details. +
+
+
+ +
+
2d. Transform inside closed details
+
+ Closed with transform inside +
+ Tiny text inside transform inside closed details. Double trouble. +
+
+
+ + +

3. Sticky and Fixed Positioning

+ +
+
3a. Sticky header inside scrollable container
+
+
+ Tiny text in a sticky header. +
+
+

Scroll this container to test sticky behavior.

+

More content to enable scrolling.

+

Even more content here.

+
Cramped text below the sticky header.
+

Additional content.

+

More filler text.

+
+
+
+ +
+
3b. Fixed-like element (within containing block)
+
+
+ Tiny text in a would-be fixed footer. +
+
+
+ + +

4. Overflow Hidden

+ +
+
4a. Content clipped by overflow:hidden
+
+

This paragraph is visible.

+ This tiny text is below the overflow cutoff, clipped but still in DOM. +

This content is also clipped away.

+
+
+ +
+
4b. overflow:hidden + transform ancestor
+
+
+

Visible inside transform + overflow.

+ Clipped tiny text inside a transformed overflow:hidden container. +
+
+
+ + +

5. Position Offsets

+ +
+
5a. Relative position with top/left offset
+
+ This tiny text is shifted via position:relative + top/left offset. +
+
+
+ +
+
5b. Absolute child in relative parent
+
+

Normal flow content at top.

+
+ Absolutely positioned tiny text at bottom-right. +
+
+
+ +
+
5c. Negative top offset
+
+
+ Tiny text pulled upward via negative top offset. +
+
+
+ + +

6. Flex and Grid Centering

+ +
+
6a. Flex-centered anti-pattern
+
+ Centered tiny text inside a flex container. +
+
+ +
+
6b. Grid-centered anti-pattern
+
+
Cramped text inside a grid-centered container.
+
+
+ +
+
6c. Flex with transform
+
+ +
+
+ + +

7. Containing Block Creators

+ +
+
7a. will-change: transform
+
+ Tiny text inside a will-change: transform container. +
+
+ +
+
7b. contain: layout
+
+ Tiny text inside a contain: layout container. +
+
+ +
+
7c. filter: brightness(1)
+
+ Tiny text inside a filter container (creates containing block). +
+
+ +
+
7d. backdrop-filter
+
+ Tiny text inside a backdrop-filter container. +
+
+ + +

8. Margin Collapse and Negative Margins

+ +
+
8a. Negative margin shifting element position
+
+
+ Tiny text pulled out of its parent via negative margins. +
+
+
+ + +

9. Combined Edge Cases

+ +
+
9a. Transform + sticky + overflow
+
+
+
+ Tiny text in sticky header inside transform inside scrollable overflow. +
+
+

Scroll content line 1.

+

Scroll content line 2.

+

Scroll content line 3.

+
Cramped box scrolled below sticky.
+

Scroll content line 4.

+

Scroll content line 5.

+
+
+
+
+ +
+
9b. Closed details + transform + flex
+
+ Closed combo scenario +
+ Triple-nested: closed details > flex > transform > tiny text. +
+
+
+ +
+
9c. Grid + absolute + transform
+
+
+ Absolute inside grid inside transform: overlays must track all three contexts. +
+
+
Absolute cramped box at bottom-right of grid.
+
+
+
+ +
+
9d. Overflow hidden + absolute breakout
+
+

Visible content.

+
+ Absolute-positioned tiny text that breaks out below overflow:hidden. +
+
+
+ +
+
9e. Side-tab card inside transform + relative offset
+
+
+
+ This card has a side-tab border accent and is offset inside a transform container. +
+
+
+
+ + + +