Conversation
…oll-wrapper tests - Increase `min-height` for mobile table cells from 1.5rem (24px) to 2.75rem (44px) to meet WCAG 2.5.5 AAA touch target recommendation - Add Playwright test to verify every table on the engines reference page is wrapped in `.table-scroll-wrapper` for consistent horizontal scrolling - Add Playwright test to verify mobile table cells meet the 44px minimum touch target size at ≤640px viewport" Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8470571e-188a-4873-926b-51b337516fdd Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves docs mobile accessibility and adds Playwright regressions to prevent future table responsiveness regressions on the engines reference page.
Changes:
- Increase mobile table-cell minimum height to meet the 44px WCAG 2.5.5 AAA touch target guidance.
- Add Playwright coverage to ensure all markdown tables on
/reference/engines/are wrapped in.table-scroll-wrapper. - Add Playwright coverage to assert mobile table-cell touch target sizing doesn’t regress.
Show a summary per file
| File | Description |
|---|---|
| docs/tests/mobile-responsive.spec.ts | Adds two Playwright tests for table scroll-wrapper coverage and mobile touch target sizing. |
| docs/src/styles/custom.css | Raises mobile table-cell min-height in the responsive card layout. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
docs/tests/mobile-responsive.spec.ts:88
- Same cleanup concern here: if navigation or the assertion fails,
await context.close()at the end won’t run and the context may leak. Use atry/finallyto guarantee closing the context (or switch to thepage/contexttest fixtures).
const context = await browser.newContext({
javaScriptEnabled: true,
viewport: { width: 390, height: 844 },
});
const page = await context.newPage();
await page.goto('/gh-aw/reference/engines/');
await page.waitForLoadState('networkidle');
- Files reviewed: 2/2 changed files
- Comments generated: 3
| const context = await browser.newContext({ | ||
| javaScriptEnabled: false, | ||
| viewport: { width: 768, height: 1024 }, | ||
| }); | ||
| const page = await context.newPage(); |
There was a problem hiding this comment.
context.close() is only reached if all assertions pass; if an expect(...) fails earlier, this browser context may leak and affect subsequent tests. Wrap the test body in try { ... } finally { await context.close(); } (or use the built-in context fixture) so cleanup always runs.
This issue also appears on line 80 of the same file.
| // Count all tables in markdown content area | ||
| const tableCount = await page.locator('.sl-markdown-content table').count(); | ||
| expect(tableCount).toBeGreaterThan(0); | ||
|
|
||
| // Count tables that are direct children of .table-scroll-wrapper | ||
| const wrappedTableCount = await page.locator('.sl-markdown-content .table-scroll-wrapper > table').count(); | ||
|
|
||
| // Every table must have a scroll wrapper for consistent horizontal scrolling on all viewports | ||
| expect(wrappedTableCount).toBe(tableCount); | ||
|
|
There was a problem hiding this comment.
This new test overlaps heavily with the existing "should wrap markdown tables in a scroll wrapper without JavaScript" test just above it. Consider merging them (e.g., keep the stronger count-based assertion and drop the weaker visibility-only check) to reduce redundancy and test runtime.
See below for a potential fix:
// The rehype plugin should have added wrapper divs at build time.
const wrapper = page.locator('.sl-markdown-content .table-scroll-wrapper').first();
await expect(wrapper).toBeVisible();
// The table must be a direct child of the wrapper.
const tableInWrapper = page.locator('.sl-markdown-content .table-scroll-wrapper > table').first();
await expect(tableInWrapper).toBeVisible();
// Every table in markdown content should be wrapped for consistent horizontal scrolling.
const tableCount = await page.locator('.sl-markdown-content table').count();
expect(tableCount).toBeGreaterThan(0);
const wrappedTableCount = await page.locator('.sl-markdown-content .table-scroll-wrapper > table').count();
| position: relative; | ||
| padding-left: 50%; | ||
| min-height: 1.5rem; | ||
| min-height: 2.75rem; |
There was a problem hiding this comment.
Using 2.75rem only guarantees a 44px minimum if the root font size is 16px; if the base font size is adjusted, the touch target could drop below 44px. To enforce the WCAG 2.5.5 44px minimum regardless of root sizing, consider min-height: 44px (or min-height: max(44px, 2.75rem) if you still want rem-based scaling).
| min-height: 2.75rem; | |
| min-height: max(44px, 2.75rem); |
🧪 Test Quality Sentinel Report
Tests Detected (Outside Scoring Scope)
Qualitative ObservationsAlthough these tests are not scored, a quick review shows they are well-structured:
Language SupportTests analyzed:
Verdict
References: §25016518533
|
There was a problem hiding this comment.
i️ Test Quality Sentinel: No score computed. Changed test files are TypeScript/Playwright specs (*.spec.ts), outside the Go/JavaScript scoring scope. The 2 new Playwright tests appear behaviorally sound — both assert observable user-facing properties (scroll-wrapper coverage, WCAG 44px touch target).
Two low-impact warnings from the multi-device docs testing report: mobile table cells fell below the WCAG 2.5.5 AAA 44 px minimum, and there was no automated check confirming every table on the engines reference page has a scroll wrapper.
Touch target fix
docs/src/styles/custom.css— inside the@media (max-width: 640px)card layout:Playwright regression tests
docs/tests/mobile-responsive.spec.ts— two new tests:count(.table-scroll-wrapper > table) === count(table)inside.sl-markdown-contenton/reference/engines/(no-JS, 768 px viewport), catching any future table that slips past the rehype plugin.getComputedStyle(td).minHeight ≥ 44on a 390 px viewport, guarding against regression of the CSS fix.