Skip to content

docs: raise mobile table-cell touch target to 44px, add scroll-wrapper coverage tests #28786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Apr 27, 2026
+47 −1
Merged

docs: raise mobile table-cell touch target to 44px, add scroll-wrapper coverage tests #28786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5db4cee
Initial plan
Copilot Apr 27, 2026
f225858
fix: increase mobile table cell touch target to 44px and add WCAG/scr…
Copilot Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/src/styles/custom.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ header::after {
text-align: right;
position: relative;
padding-left: 50%;
min-height: 1.5rem;
min-height: 2.75rem;
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
min-height: 2.75rem;
min-height: max(44px, 2.75rem);

Copilot uses AI. Check for mistakes.
}

.sl-markdown-content td:first-child {
Expand Down
46 changes: 46 additions & 0 deletions docs/tests/mobile-responsive.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,52 @@ test.describe('Mobile and Responsive Layout', () => {
await context.close();
});

test('should wrap ALL markdown tables in a scroll wrapper on the engines reference page', async ({ browser }) => {
const context = await browser.newContext({
javaScriptEnabled: false,
viewport: { width: 768, height: 1024 },
});
const page = await context.newPage();
Comment on lines +57 to +61
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

await page.goto('/gh-aw/reference/engines/');
await page.waitForLoadState('domcontentloaded');

// 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);

Comment on lines +66 to +75
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copilot uses AI. Check for mistakes.
await context.close();
});

test('should have WCAG 2.5.5-compliant touch target size for mobile table cells', async ({ browser }) => {
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');

// On mobile (<=640px), table cells are rendered as stacked cards.
// Each cell must meet the WCAG 2.5.5 AAA minimum touch target of 44 px (2.75 rem).
const tdMinHeight = await page.evaluate(() => {
const td = document.querySelector('.sl-markdown-content table tbody td');
if (!td) return 0;
return parseFloat(getComputedStyle(td).minHeight);
});

expect(tdMinHeight).toBeGreaterThanOrEqual(44);

await context.close();
});

for (const formFactor of formFactors) {
test.describe(`${formFactor.name}`, () => {
test.beforeEach(async ({ page }) => {
Expand Down
Loading