Skip to content

The dismissal test waits for the frame, not the popup #430

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

Open
serenakeyitan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

The dismissal test waits for the frame, not the popup #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion server/frame-probe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@
// While something is open, the next click anywhere in the document only
// dismisses it — it must not open a different comment or start a new one.
var shellUiOpen = false;
// Mirrored onto <html> alongside the other frame state. The shell unmounts a
// card at commit and only tells us one effect + one postMessage later, so
// "the popup is gone from the shell" and "the frame will act on the next
// click again" are two different moments. Anything waiting on the second one
// has to be able to see it.
function setShellUiOpen(open) {
shellUiOpen = open;
if (open) document.documentElement.setAttribute('data-tdoc-ui-open', '');
else document.documentElement.removeAttribute('data-tdoc-ui-open');
}
var swallowClick = false;
var dismissDownX = 0, dismissDownY = 0;
var COMMENT_ICON_PATH = 'M2 2H12A10 10 0 1 1 2 12V2Z';
Expand Down Expand Up @@ -1064,7 +1074,7 @@
}
else if (d.type === 'tdoc:theme') applyTheme(d.theme);
else if (d.type === 'tdoc:mode') setInteractionMode(d.mode);
else if (d.type === 'tdoc:uiOpen') shellUiOpen = !!d.open;
else if (d.type === 'tdoc:uiOpen') setShellUiOpen(!!d.open);
else if (d.type === 'tdoc:editFormat') formatEdit(d.command, d.value);
else if (d.type === 'tdoc:editRestore') {
var restoreRoot = findEditRoot();
Expand Down
19 changes: 17 additions & 2 deletions test/browser-editing.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,14 @@ async function chooseMode(page, label) {
};
});
const openCount = () => page.locator('.tdoc-popup, .tdoc-margin-comment.active').count();
// The frame, not the shell, decides whether a click dismisses or acts, and
// it decides from its own copy of the open state — which arrives one
// effect and one postMessage after the shell has already mounted or
// unmounted the card. openCount() is the shell's half of that; a click
// sent between the two halves is read as a dismissal and never opens the
// composer, which is what made this test fail one run in four.
const frameKnows = (open) => frame.waitForFunction(
(want) => document.documentElement.hasAttribute('data-tdoc-ui-open') === want, open);
const clickAt = async (point) => {
await page.mouse.click(box.x + point.x, box.y + point.y);
await page.waitForTimeout(250);
Expand All @@ -296,22 +304,25 @@ async function chooseMode(page, label) {
// 1. A drag while something is open is a selection, not a dismissal.
// Clearing on mousedown unmounted the focused composer, and losing
// that focus wiped the selection mid-drag.
await frameKnows(false);
await clickAt(geometry.first);
assert(await openCount() > 0, 'precondition: a word click did not open the composer');
await frameKnows(true);
await page.mouse.move(box.x + geometry.drag.x1, box.y + geometry.drag.y);
await page.mouse.down();
await page.mouse.move(box.x + geometry.drag.x2, box.y + geometry.drag.y, { steps: 12 });
await page.mouse.up();
await page.waitForTimeout(400);
assert(await openCount() > 0, 'a drag while something was open reported no selection');
await page.locator('.tdoc-popup button.x').click().catch(() => {});
await page.waitForTimeout(200);
await frameKnows(false);

// 2. The artifact pill is our own UI, but while a card is open it is
// outside that card like anything else: it dismisses, it does not
// open an element comment.
await clickAt(geometry.first);
assert(await openCount() > 0, 'precondition: composer did not reopen');
await frameKnows(true);
// Dispatch the hover inside the frame: Playwright's own hover() checks
// actionability against the top document, where the open composer sits
// over the artifact and the check never settles.
Expand All @@ -323,9 +334,13 @@ async function chooseMode(page, label) {
}));
});
await frame.locator('.tdoc-comment-pill').waitFor({ state: 'visible' });
// boundingBox() is already in main-frame coordinates for an element inside
// an iframe. Adding the doc-frame offset a second time put this click 48px
// below the pill, on plain prose — which also dismisses, so the assertion
// passed without ever touching the pill it is named after.
const pill = await frame.locator('.tdoc-comment-pill').boundingBox();
assert(pill, 'hovering the artifact did not show the comment pill');
await page.mouse.click(box.x + pill.x + pill.width / 2, box.y + pill.y + pill.height / 2);
await page.mouse.click(pill.x + pill.width / 2, pill.y + pill.height / 2);
await page.waitForTimeout(400);
assert(await openCount() === 0, 'clicking the artifact pill opened a comment instead of dismissing');

Expand Down