Skip to content

Navigation Menu

Sign in
Sign up

fix(lab): harden Drawer layer lifecycle #6073

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.

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

Draft
imdreamrunner wants to merge 1 commit into main
base: main
Choose a base branch
Loading
from agent/harden-lab-drawer-20260904
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
225 changes: 167 additions & 58 deletions .github/scripts/modal-close-visibility.js
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,37 @@
#!/usr/bin/env node
// Copyright (c) Meta Platforms, Inc. and affiliates.


/**
* @description Asserts native-dialog close/render ordering in a real browser
* @description Asserts native Drawer host exit/render/dismissal ordering in a real browser
* @input --storybook-dir <path> [--port <n>]
* @output One line per target; exit 1 if a close/render invariant fails
* @output One line per target; exit 1 if a native-host invariant fails
*
* Two opposite ordering bugs live at this boundary:
* Four ordering contracts live at this boundary:
*
* 1. A modal <dialog> that is `display: none` while still `:modal` blocks every
* pointer event on the document, and Safari 26.1 did not release that block
* when close() finally ran (#4290). It must still be rendered when close()
* starts.
* 2. A fixed panel that stays rendered after close() leaves the top layer can
* paint back inside a transformed ancestor for one frame (#5549). It must be
* hidden before the next paint after close().
* 2. A fixed panel that stays rendered after its native host leaves the top
* layer can paint back inside a transformed ancestor for one frame (#5549).
* It must be hidden before the next paint.
* 3. The non-modal Popover host must preserve `<dialog>`'s observable `close`
* event so ref/onClose consumers keep their focus-restoration contract.
* 4. A closing top drawer stays on the shared dismissal stack until its visual
* exit completes, so a second Escape cannot dismiss the drawer below it.
*
* Neither ordering exists in jsdom: it runs no CSS transition and has no top
* layer. Chromium is enough to observe both invariants; the browser-specific
* None of these orderings exists in jsdom: it runs no CSS transition and has no
* top layer. Chromium is enough to observe the invariants; the browser-specific
* failure that first exposed each one is not needed to see the ordering fail.
*/

const {chromium} = require('playwright');
const {chromium} = require('playwright');
const fs = require('node:fs');
const path = require('node:path');
const http = require('node:http');

const args = process.argv.slice(2);
const getArg = (name) => {
const getArg = name => {
const idx = args.indexOf(`--${name}`);
return idx !== -1 ? args[idx + 1] : null;
};
Expand All @@ -43,14 +46,33 @@ const TARGETS = [
openButton: 'Open Navigation',
},
{
component: 'Drawer',
component: 'Drawer (modal)',
story: 'lab-drawer--showcase',
openButton: 'Open inspector',
// Reproduces the real failure condition: after close() releases the top
// layer, this becomes the containing block for the fixed panel.
// Reproduces the real failure condition: after the native host releases the
// top layer, this becomes the containing block for the fixed panel.
transformAncestor: true,
mustBeHiddenAfterClose: true,
},
{
component: 'Drawer (non-modal)',
story: 'lab-drawer--row-inspector',
openButton: 'web-01 / us-east-1',
host: 'popover',
transformAncestor: true,
mustBeHiddenAfterClose: true,
mustDispatchClose: true,
},
{
component: 'Drawer (stacked exit)',
story: 'lab-drawer--stacked-drawers',
openButton: 'Open order',
nestedButton: 'Open line item',
host: 'popover',
stackedExit: true,
mustBeHiddenAfterClose: true,
mustDispatchClose: true,
},
];

const CONTENT_TYPES = {
Expand All @@ -63,7 +85,7 @@ const CONTENT_TYPES = {
};

function createServer(dir, listenPort) {
return new Promise((resolve) => {
return new Promise(resolve => {
const server = http.createServer((req, res) => {
const filePath = path
.join(dir, req.url === '/' ? 'index.html' : req.url)
Expand All @@ -83,8 +105,7 @@ function createServer(dir, listenPort) {
return;
}
res.writeHead(200, {
'Content-Type':
CONTENT_TYPES[path.extname(resolved)] || 'text/plain',
'Content-Type': CONTENT_TYPES[path.extname(resolved)] || 'text/plain',
});
res.end(data);
});
Expand All @@ -94,36 +115,65 @@ function createServer(dir, listenPort) {
});
}

// Records the computed `display` of every dialog at the instant it is closed.
function recordCloseDisplay() {
// Records the rendered state at the instant either native host is released.
function recordHostExitDisplay() {
window.__closeDisplays = [];
window.__postCloseFrames = [];
const close = HTMLDialogElement.prototype.close;
HTMLDialogElement.prototype.close = function (...args) {
const dialog = this;
window.__closeDisplays.push(getComputedStyle(dialog).display);
const result = close.apply(dialog, args);
// Sample immediately before the next paint. Drawer must have committed its
// hide by this point; otherwise the non-top-layer panel can paint against a
// transformed ancestor for one frame.
window.__dialogCloseEvents = 0;

const sampleAfterExit = dialog => {
requestAnimationFrame(() => {
const rect = dialog.getBoundingClientRect();
window.__postCloseFrames.push({
display: getComputedStyle(dialog).display,
open: dialog.open,
popoverOpen: dialog.matches(':popover-open'),
x: Math.round(rect.x),
width: Math.round(rect.width),
});
});
};
const recordExit = (dialog, exit, args) => {
window.__closeDisplays.push(getComputedStyle(dialog).display);
const result = exit.apply(dialog, args);
// Sample immediately before the next paint. Drawer must have committed its
// hide by this point; otherwise the non-top-layer panel can paint against a
// transformed ancestor for one frame.
sampleAfterExit(dialog);
return result;
};

const close = HTMLDialogElement.prototype.close;
HTMLDialogElement.prototype.close = function (...args) {
return recordExit(this, close, args);
};

const hidePopover = HTMLElement.prototype.hidePopover;
if (typeof hidePopover === 'function') {
HTMLElement.prototype.hidePopover = function (...args) {
if (this instanceof HTMLDialogElement) {
return recordExit(this, hidePopover, args);
}
return hidePopover.apply(this, args);
};
}

document.addEventListener(
'close',
event => {
if (event.target instanceof HTMLDialogElement) {
window.__dialogCloseEvents += 1;
}
},
true,
);
}

async function probe(page, target) {
await page.addInitScript(recordCloseDisplay);
await page.addInitScript(recordHostExitDisplay);
await page.goto(
`http://localhost:${port}/iframe.html?id=${target.story}&viewMode=story`,
{waitUntil: 'networkidle', timeout: 15000 }
{waitUntil: 'networkidle', timeout: 15000},
);

if (target.transformAncestor) {
Expand All @@ -136,27 +186,65 @@ async function probe(page, target) {
});
}

await page.getByRole('button', { name: target.openButton }).click();
const hostSelector =
target.host === 'popover' ? 'dialog:popover-open' : 'dialog:modal';
const openHostCount = target.stackedExit ? 2 : 1;
await page.getByRole('button', {name: target.openButton}).click();
if (target.nestedButton) {
await page.getByRole('button', {name: target.nestedButton}).click();
}
await page.waitForFunction(
() => document.querySelector('dialog')?.matches(':modal') === true,
null,
{timeout: 5000 }
({selector, count}) => document.querySelectorAll(selector).length === count,
{selector: hostSelector, count: openHostCount},
{timeout: 5000},
);

let stackedExitOwned = true;
await page.keyboard.press('Escape');
await page.waitForFunction(() => window.__closeDisplays.length > 0, null, {
timeout: 5000,
});
if (target.stackedExit) {
// The inner drawer is still visibly/top-layer present during its slide-out.
// A second Escape must stay with it rather than dismissing the outer drawer.
await page.keyboard.press('Escape');
await page.waitForTimeout(50);
stackedExitOwned =
(await page.locator(hostSelector).count()) === openHostCount;
await page.waitForFunction(
selector => document.querySelectorAll(selector).length === 1,
hostSelector,
{timeout: 5000},
);
await page.keyboard.press('Escape');
}

const expectedExits = target.stackedExit ? 2 : 1;
await page.waitForFunction(
count => window.__closeDisplays.length >= count,
expectedExits,
{timeout: 5000},
);
await page.waitForFunction(
selector => document.querySelectorAll(selector).length === 0,
hostSelector,
{timeout: 5000},
);
if (target.mustBeHiddenAfterClose) {
await page.waitForFunction(() => window.__postCloseFrames.length > 0, null, {
timeout: 5000,
});
await page.waitForFunction(
count => window.__postCloseFrames.length >= count,
expectedExits,
{timeout: 5000},
);
}

return page.evaluate(() => ({
closeDisplays: window.__closeDisplays,
postCloseFrames: window.__postCloseFrames,
}));
return page.evaluate(
({expectedExits, stackedExitOwned}) => ({
closeDisplays: window.__closeDisplays,
postCloseFrames: window.__postCloseFrames,
closeEvents: window.__dialogCloseEvents,
expectedExits,
stackedExitOwned,
}),
{expectedExits, stackedExitOwned},
);
}

async function run() {
Expand All @@ -172,34 +260,55 @@ async function run() {

try {
const context = await browser.newContext({
viewport: {width: 430, height: 860},
viewport: {width: 430, height: 860},
});

for (const target of TARGETS) {
const page = await context.newPage();
try {
const { closeDisplays, postCloseFrames } = await probe(page, target);
const hiddenAtClose = closeDisplays.filter((d) => d === 'none');
const paintedAfterClose = target.mustBeHiddenAfterClose
? postCloseFrames.filter((frame) => frame.display !== 'none')
const {
closeDisplays,
postCloseFrames,
closeEvents,
expectedExits,
stackedExitOwned,
} = await probe(page, target);
const hiddenAtExit = closeDisplays.filter(d => d === 'none');
const paintedAfterExit = target.mustBeHiddenAfterClose
? postCloseFrames.filter(frame => frame.display !== 'none')
: [];
const missingCloseEvent =
target.mustDispatchClose && closeEvents < expectedExits;

if (hiddenAtClose.length > 0) {
if (!stackedExitOwned) {
failures++;
console.error(
`✗ ${target.component}: a second Escape reached the lower drawer during the top drawer's exit`,
);
} else if (hiddenAtExit.length > 0) {
failures++;
console.error(
`✗ ${target.component}: native host exited at display: none (${closeDisplays.join(', ')})`,
);
} else if (paintedAfterExit.length > 0) {
failures++;
console.error(
`✗ ${target.component}: close() ran at display: none (${closeDisplays.join(', ')})`
`✗ ${target.component}: painted after native host exit — ${JSON.stringify(paintedAfterExit)}`,
);
} else if (paintedAfterClose.length > 0) {
} else if (missingCloseEvent) {
failures++;
console.error(
`✗ ${target.component}: painted after close() outside the top layer — ${JSON.stringify(paintedAfterClose)}`
`✗ ${target.component}: emitted ${closeEvents}/${expectedExits} dialog close event(s)`,
);
} else {
const postClose = target.mustBeHiddenAfterClose
? '; hidden before the next paint'
: '';
const closeEvent = target.mustDispatchClose
? `; ${closeEvents} close event(s)`
: '';
console.log(
`✓ ${target.component}: close() ran at display: ${closeDisplays.join(', ')}${postClose}`
`✓ ${target.component}: native host exited at display: ${closeDisplays.join(', ')}${postClose}${closeEvent}`,
);
}
} catch (e) {
Expand All @@ -216,19 +325,19 @@ async function run() {

if (failures > 0) {
console.error(
`\nFailing: ${failures} native-dialog close/render ordering check(s) — see #4290 and #5549.`
`\nFailing: ${failures} native-host Drawer invariant check(s) — see #4290 and #5549.`,
);
return 1;
}
console.log('\nAll native-dialog close/render ordering checks passed.');
console.log('\nAll native-host Drawer invariant checks passed.');
return 0;
}

run()
.then((code) => {
.then(code => {
process.exitCode = code;
})
.catch((e) => {
console.error('Native-dialog close visibility guard failed:', e);
.catch(e => {
console.error('Native-host Drawer guard failed:', e);
process.exit(1);
});
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ export default function ComponentScoresPage() {

// Switching rows keeps the non-modal drawer open. Reset its reading
// position and move focus to the new heading so the changed audit is
// announced and Escape continues to reach the drawer's key handler.
// announced and Escape continues to reach the shared dismissal owner.
useEffect(() => {
if (!isAuditPanelOpen || !selectedRowId) {
return;
Expand Down
Loading
Loading

AltStyle によって変換されたページ (->オリジナル) /