From e7885ce1eb5857b0118ea455919665debcdd06e7 Mon Sep 17 00:00:00 2001 From: Andreas Reinhold / reini Date: Fri, 18 Sep 2026 09:49:17 +0200 Subject: [PATCH] Keep the search view closed when its focus comes back late MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a slow machine Escape could close the full-screen search and have it open again for good. A close hands focus back to the input — Escape does, and so does the view's focus trap as it lets go — and `focused()` told that return from someone coming to search by a 250ms wall-clock window. Both returns run on frames, and a runner painting a few frames a second took longer than that, so the returning focus opened the view again. A `returning` flag now covers the close until the hand-back has actually run, on the same frame, and the constant is gone. `hold()` times the full-screen layout from the exit's own duration token, the one Alpine's x-transition holds `display` for, instead of waiting a frame or two for the exit's transitions to appear: `getAnimations()` is empty both before an engine creates them and after they end, and a loaded engine can leave a second between frames. The browser tests were racing the same slow machine, reproduced in a Linux container like the runner, with its Playwright Firefox and WebKitGTK and two cores kept busy: - The browser plugin retries every script and action with a one-second attempt until the budget runs out. An in-page sleep longer than that only ever passed on the last attempt, which is where the 47-50s tests came from, and a script that clicks was run again against a page that had moved on. `onceInPage()` runs such a script once however often it is retried; the long sleeps are plain retried conditions now. - Under load WebKitGTK paints no frame while a tight setTimeout loop runs, so a sample loop saw the start value and then nothing. `caughtMidExit()` samples on animation frames, for 2.5s. - "No animations running" is also true before an opening transition exists, so a close could be sampled from a scrim at 4% opacity. `settled()` waits two frames before it asks. The workflow no longer uploads failure screenshots: Gitea's artifact service timed out on every attempt, two minutes per red run, and the job logs are readable without it. Feature 1159 passed. Browser 299 passed on Chrome, Firefox and WebKit on macOS, and on Firefox and WebKitGTK in the Linux container under load, twice each. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/tests.yml | 11 --- resources/js/search.js | 81 +++++++++---------- tests/Browser/ActionsTest.php | 13 ++- tests/Browser/ContainmentTest.php | 127 +++++++++++++++--------------- tests/Browser/DatepickerTest.php | 19 ++++- tests/Browser/NavigationTest.php | 43 +++------- tests/Browser/PickingTest.php | 75 ++++++++++-------- tests/Pest.php | 89 +++++++++++++++++++++ 8 files changed, 270 insertions(+), 188 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e53586e6..9b879fbd 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -103,14 +103,3 @@ jobs: env: BROWSER_TIMEOUT: 45000 - # What the runner saw when an assertion or an action gave up. A failure that does not - # reproduce on a workstation is otherwise only a stack trace: the page itself says whether an - # element was covered, unstyled or never drawn. - - name: Keep the screenshots of what failed - if: failure() - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 - with: - name: screenshots-${{ matrix.browser }} - path: tests/Browser/Screenshots - if-no-files-found: ignore - retention-days: 7 diff --git a/resources/js/search.js b/resources/js/search.js index fb877172..f606567c 100644 --- a/resources/js/search.js +++ b/resources/js/search.js @@ -24,19 +24,15 @@ * transition has run, while the header bar fades out with it (search.css). Without it the bar went * back to its resting form, or to nothing behind the icon, on the first frame of the exit, and the * fading view dropped to the docked layout. The focus trap lets go at the close itself, not at the - * end of the exit, so its return of focus still lands inside RETURN_GUARD_MS. + * end of the exit, so `returning` (close()) covers its return of focus like the field's own. */ import { upTo } from './breakpoints.js' +import { ms } from './util.js' const FOCUSABLE = 'a[href], button:not([disabled]), input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])' const CHOOSES = 'a[href], button:not([disabled]), [data-md-list-open]' -// A close hands focus back to the input: Escape does, and so does the full-screen view's focus trap -// when it lets go, a moment later. Focus arriving this soon after a close is that, not someone -// coming to search. -const RETURN_GUARD_MS = 250 - // A Livewire morph replaces the results in several mutations; wait for the batch to end before // counting, so the live region speaks once. const SETTLE_MS = 120 @@ -48,7 +44,8 @@ document.addEventListener('alpine:init', () => { leaving: false, leavings: 0, compact: false, - closedAt: -Infinity, + // A close is handing focus back; see close(). + returning: false, announcement: '', // What is in the field, which is what tells suggestions from results. query: '', @@ -126,64 +123,64 @@ document.addEventListener('alpine:init', () => { }, focused() { - if (performance.now() - this.closedAt > RETURN_GUARD_MS) { + if (!this.returning) { this.show() } }, + /** + * `refocus` puts focus back where the view came from: the icon button, or the field itself. + * + * Either way a close hands focus back — the full-screen view's focus trap returns it as it + * lets go, whether this asked for it or not — and that focus must not read as someone + * coming to search, or the view would open again on its way out. `returning` covers it + * until the hand-back has run, rather than for a fixed stretch of wall-clock time after + * the close: both returns are scheduled on frames, and a runner painting a handful of + * frames a second takes longer over one than any such guard would allow, which left the + * view open for good. + */ close(refocus = false) { const fromFullScreen = this.open && this.fullScreen + const back = refocus ? (this.$refs.trigger ?? this.$refs.input) : null this.open = false - this.closedAt = performance.now() + this.returning = true if (fromFullScreen) { this.hold() } - if (refocus) { - // Back to whatever opened the view: the icon button, or the field itself. A frame - // after the tick, as `expand()` waits: the icon button is `x-show`n, and Alpine only - // shows it on that frame, so a focus in the tick reaches a hidden button, which - // Firefox and WebKit refuse (in Chrome the trap's own return had covered for it). - const back = this.$refs.trigger ?? this.$refs.input - - this.$nextTick(() => requestAnimationFrame(() => back.focus())) - } + // A frame after the tick, as `expand()` waits: the icon button is `x-show`n, and Alpine + // only shows it on that frame, so a focus in the tick reaches a hidden button, which + // Firefox and WebKit refuse (in Chrome the trap's own return had covered for it). + this.$nextTick(() => requestAnimationFrame(() => { + back?.focus() + this.returning = false + })) }, /** - * Keeps the full-screen layout for the length of the view's exit: until the animations the - * closed state starts, a frame on, have finished (at once when none run); reopening, which - * counts `leavings` up, lets a pending end go by. + * Keeps the full-screen layout for the length of the view's exit — the duration search.css + * gives it, which is also the one Alpine's `x-transition` holds its `display` for, so the + * two end together. Reopening, which counts `leavings` up, lets a pending end go by, and + * reduced motion zeroes the token, which ends the hold on the next task, as it should. + * + * The duration rather than the exit's own animations: `getAnimations()` is empty both + * before an engine has created the transitions and after they have finished, and nothing + * on the element tells the two apart. Waiting a frame or two for them to appear only moves + * the guess — a loaded engine can leave a second between two frames, and holding for what + * it had not started yet ended the full-screen layout at once, mid-exit. */ hold() { const leaving = ++this.leavings this.leaving = true - // A frame on, the closed state has met the style and the exit's transitions exist, as - // the rail's own settle() reads them. One frame is not always enough: an engine that - // starts them on its next refresh tick would show none here, and holding for nothing - // would end the full-screen layout at once, mid-exit. So an empty list is asked again - // on the following frame before it counts as "nothing to wait for". - const hold = (frame) => requestAnimationFrame(() => { - const animations = this.$refs.view?.getAnimations() ?? [] - - if (animations.length === 0 && frame === 0) { - hold(1) - - return + setTimeout(() => { + if (leaving === this.leavings) { + this.leaving = false } - - Promise.allSettled(animations.map((animation) => animation.finished)).then(() => { - if (leaving === this.leavings) { - this.leaving = false - } - }) - }) - - hold(0) + }, ms(this.$refs.view, '--md-sys-motion-spatial-fast-duration') ?? 0) }, clear() { diff --git a/tests/Browser/ActionsTest.php b/tests/Browser/ActionsTest.php index 7acf7c9a..8b1fafa6 100644 --- a/tests/Browser/ActionsTest.php +++ b/tests/Browser/ActionsTest.php @@ -906,10 +906,13 @@ it('opens a sheet-at-compact menu below 600px, focused on its first item, and th ->assertScript(focused("textContent.trim() === 'Photo'")); // From 600px the trigger opens the popover instead, and a resize across it while one is open - // closes whichever was shown. + // closes whichever was shown. `aria-haspopup` is how the trigger says what it opens now, and + // the menu writes it in the same handler as that close — so waiting for it is waiting for the + // component to have heard of the new width. A press that lands before the media query's change + // event is delivered, which a loaded runner leaves room for, is closed again by it a moment on. $page->resize(900, 800) - ->click($trigger) ->assertAttribute($trigger, 'aria-haspopup', 'menu') + ->click($trigger) ->assertAttribute($trigger, 'aria-expanded', 'true'); $page->resize(400, 800) @@ -1023,8 +1026,10 @@ it('fades a menu out after the browser has closed it, on Escape or a press outsi ->assertScript(EXIT_COPY_FADING) // The copy is decoration: Alpine starts nothing inside it. ->assertScript("window.eval(\"[...document.querySelectorAll('[data-md-popover-ghost], [data-md-popover-ghost] *')].every((element) => element._x_dataStack === undefined)\")") - // Gone once the slowest of its transitions has run. - ->assertScript('new Promise((resolve) => setTimeout(() => resolve(document.querySelector("[data-md-popover-ghost]") === null), 3300))'); + // Gone once the slowest of its transitions has run: assertScript's own retry waits for it, + // where sleeping for the exit's length inside the page outlived the plugin's 1000ms budget + // for one script attempt and had the whole expectation run again until it timed out. + ->assertScript('document.querySelector("[data-md-popover-ghost]") === null'); // A press outside it. $page->click(MORE) diff --git a/tests/Browser/ContainmentTest.php b/tests/Browser/ContainmentTest.php index c16372f8..bc06a5d0 100644 --- a/tests/Browser/ContainmentTest.php +++ b/tests/Browser/ContainmentTest.php @@ -442,14 +442,18 @@ it('cycles a bottom sheet\'s preset heights from its handle, announcing each, an ->assertScript("{$sheet}.style.getPropertyValue('--sheet-stop').trim() === '50dvh'") // Settled, not still sliding in: a grip pressed while the entry is still under way would // read as unstable on a loaded runner, the way opening it does elsewhere in this file. - ->assertScript("{$sheet}.getAnimations({ subtree: true }).length === 0"); + ->assertScript(settled($sheet, subtree: true)); $gripSelector = '#containment [data-md-bottom-sheet-panel][data-md-preset] [data-md-bottom-sheet-grip]'; $page->click($gripSelector); $page->assertScript("{$sheet}.style.getPropertyValue('--sheet-stop').trim() === '90dvh'") - ->assertScript("{$announce}.textContent.trim() === 'Height 3 of 3'"); + ->assertScript("{$announce}.textContent.trim() === 'Height 3 of 3'") + // The stop is written before the sheet has grown to it, so wait for the growth as well as + // for the entry above: the grip travels with the sheet's edge, and a press that lands + // while it is moving never reads as stable. + ->assertScript(settled($sheet, subtree: true)); // From the last stop, activating the handle closes the sheet, as a handle with no stops does. $page->click($gripSelector); @@ -519,35 +523,32 @@ it('gives a closing standard side sheet\'s room back to the content beside it as // The example starts open, and stands open from the first frame Alpine draws: full 400px, fully // shown, nothing on it animating. $page = containment()->resize(1000, 800) - ->assertScript("{$root}.hasAttribute('data-md-open') && Math.round({$root}.getBoundingClientRect().width) === 400 && getComputedStyle({$sheet}).opacity === '1' && {$root}.getAnimations({ subtree: true }).length === 0"); + ->assertScript("{$root}.hasAttribute('data-md-open') && Math.round({$root}.getBoundingClientRect().width) === 400 && getComputedStyle({$sheet}).opacity === '1'") + ->assertScript(settled($root, subtree: true)) + // Only once the view has settled, two frames after Alpine starts, which WebKit can still + // be waiting for here: until then the sheet has no transitions, on purpose, and closes at + // once. + ->assertScript("{$root}.hasAttribute('data-md-drawer-settled')"); // Closed and sampled in one round trip: the sheet's root caught part-way between its width // and none, still in the layout, while the column beside it has grown part of the way — not - // the whole sheet and its gap handed back in one jump at the end. Only once the view has - // settled, two frames after Alpine starts, which WebKit can still be waiting for here: until - // then the sheet has no transitions, on purpose, and closes at once. - $midExit = $page->script(<< { - const root = {$root} - const column = {$column} + // the whole sheet and its gap handed back in one jump at the end. + $midExit = $page->script(caughtMidExit( + << { + const width = root.getBoundingClientRect().width + const columnWidth = column.getBoundingClientRect().width - for (let i = 0; i < 100 && ! root.hasAttribute('data-md-drawer-settled'); i++) { - await new Promise((resolve) => setTimeout(resolve, 10)) - } - - const open = { root: root.getBoundingClientRect().width, column: column.getBoundingClientRect().width, row: root.parentElement.getBoundingClientRect().width } - {$toggle}.click() - - for (let i = 0; i < 80; i++) { - const width = root.getBoundingClientRect().width - const columnWidth = column.getBoundingClientRect().width - - if (! root.hasAttribute('data-md-drawer-collapsed') && getComputedStyle(root).display !== 'none' && width > 1 && width < open.root - 1 && columnWidth > open.column + 1 && columnWidth < open.row - 1) return true - await new Promise((resolve) => setTimeout(resolve, 5)) - } - return false + return ! root.hasAttribute('data-md-drawer-collapsed') && getComputedStyle(root).display !== 'none' && width > 1 && width < open.root - 1 && columnWidth > open.column + 1 && columnWidth < open.row - 1 })() - JS); + JS, + )); expect($midExit)->toBeTrue(); @@ -1001,28 +1002,24 @@ it('fades the sheet\'s scrim out on close, rather than making it vanish', functi $page = containment() ->click($trigger) ->assertScript("getComputedStyle({$sheet}).display !== 'none'") - ->assertScript("getComputedStyle({$scrim}).opacity === '1'"); + ->assertScript("getComputedStyle({$scrim}).opacity === '1'") + // Settled, not still fading in: an engine reads a transition's end value until its next + // refresh tick, so an opacity of 1 alone does not say the entry has run — and a close + // sampled from there has nothing left to fade. + ->assertScript(settled($scrim)); slowMotion($page); // Triggering the close and sampling for a mid-fade opacity in the same round trip: a round // trip apiece for a separate `script` and `assertScript` already costs real time, easily as - // much as the 200ms fade itself, so a click, then a later separate read, can just as easily - // land before the fade starts or after it ends. This samples every few ms, in the page itself, - // for whether it was ever caught strictly between fully shown and fully hidden, rather than - // simply gone at once. - $midFade = $page->script(<< { - {$scrim}.click() - - for (let i = 0; i < 60; i++) { - const opacity = parseFloat(getComputedStyle({$scrim}).opacity) - if (opacity > 0.02 && opacity < 0.98) return true - await new Promise((resolve) => setTimeout(resolve, 5)) - } - return false - })() - JS); + // much as the fade itself, so a click, then a later separate read, can just as easily land + // before the fade starts or after it ends. This samples once a frame, in the page itself, for + // whether it was ever caught strictly between fully shown and fully hidden, rather than simply + // gone at once (caughtMidExit() in tests/Pest.php). + $midFade = $page->script(caughtMidExit( + "{$scrim}.click()", + "(() => { const opacity = parseFloat(getComputedStyle({$scrim}).opacity); return opacity > 0.02 && opacity < 0.98 })()", + )); expect($midFade)->toBeTrue(); @@ -1037,31 +1034,32 @@ it('slides the sheet out on close, rather than making it vanish', function (stri $page = containment() ->click($trigger) ->assertScript("getComputedStyle({$sheet}).display !== 'none'") - ->assertScript("Math.abs({$offset}) < 0.5"); + ->assertScript("Math.abs({$offset}) < 0.5") + // Settled, not still sliding in: see the scrim's fade above. + ->assertScript(settled($sheet)); slowMotion($page); // Sampled in the page, in the same round trip as the close, as the scrim's fade is above: the // sheet must be caught still displayed and part of the way to its closed offset (one sheet's // own size past its edge), not gone at once — which is what Firefox showed while the exit - // leaned on `allow-discrete` holding `display`. 400 iterations (2000ms): the sheet closes on - // the emphasized-accelerate easing, which stays close to its start for a good part of its run, - // so slowMotion()'s stretched exit needs longer than a scrim's fade (a spring, front-loaded) - // before the offset has moved past a pixel. - $midSlide = $page->script(<< { - const size = {$sheet}.getBoundingClientRect().{$dimension} - {$scrim}.click() + // leaned on `allow-discrete` holding `display`. The sheet closes on the emphasized-accelerate + // easing, which stays close to its start for a good part of its run, so it takes a few of the + // stretched exit's frames before the offset has moved past a pixel. + $midSlide = $page->script(caughtMidExit( + << { + const style = getComputedStyle({$sheet}) + const offset = {$sample} - for (let i = 0; i < 400; i++) { - const style = getComputedStyle({$sheet}) - const offset = {$sample} - if (style.display !== 'none' && offset > 1 && offset < size - 1) return true - await new Promise((resolve) => setTimeout(resolve, 5)) - } - return false + return style.display !== 'none' && offset > 1 && offset < size - 1 })() - JS); + JS, + )); expect($midSlide)->toBeTrue(); @@ -1498,7 +1496,8 @@ it('eases a collapse bound to Alpine open and shut, keeping the binding in step' it('turns a collapse round from the height it has reached, ending as the last press asked', function () { $page = collapseMotionProbe('1500ms'); - $result = $page->script(<<<'JS' + // Two seconds in the page, so it is run once: see onceInPage() in tests/Pest.php. + $result = $page->script(onceInPage(<<<'JS' (async () => { const details = document.querySelector('#plain') const summary = details.querySelector('summary') @@ -1516,7 +1515,7 @@ it('turns a collapse round from the height it has reached, ending as the last pr return { closed, midway, turned, end: details.getBoundingClientRect().height, open: details.open, style: details.getAttribute('style') } })() - JS); + JS)); expect($result['midway'])->toBeGreaterThan($result['closed'] + 2) // Turned round from where it was, not from either end. @@ -1531,7 +1530,7 @@ it('closes the open member of a name group on the same spring as the one opening $first = "document.querySelector('#first')"; - $result = $page->script(<<<'JS' + $result = $page->script(onceInPage(<<<'JS' (async () => { const first = document.querySelector('#first') const second = document.querySelector('#second') @@ -1550,7 +1549,7 @@ it('closes the open member of a name group on the same spring as the one opening return { stillOpen, caught, first: first.open, second: second.open, name: first.getAttribute('name') } })() - JS); + JS)); expect($result)->toBe(['stillOpen' => true, 'caught' => true, 'first' => false, 'second' => true, 'name' => 'faq']); diff --git a/tests/Browser/DatepickerTest.php b/tests/Browser/DatepickerTest.php index 828093fc..901e1adc 100644 --- a/tests/Browser/DatepickerTest.php +++ b/tests/Browser/DatepickerTest.php @@ -538,9 +538,13 @@ it('opens the range picker full screen below 600px and grows its month list both $page = dateProbe() ->resize(390, 800) // Explicit, not implicit: the resize's own reflow can still be under way when the click - // that follows fires, which is what made this time out on a loaded runner — the toggle's - // box kept moving under the click rather than the click itself being slow. - ->assertScript('window.innerWidth === 390 && document.documentElement.getAnimations({ subtree: true }).length === 0'); + // that follows fires, and the press it costs the runner is the one press this test cannot + // afford to lose. The browser plugin gives each attempt a second and then presses again, + // and once the full-screen picker is up it covers the toggle it came from — the month + // grid is what the browser finds there — so no later attempt can ever land. That is the + // 90 seconds (45s of attempts, then one last one with the whole budget) this timed out in. + ->assertScript('window.innerWidth === 390') + ->assertScript(settled('document.documentElement', subtree: true)); expect($page->script(hitTarget('[aria-controls=\"trip-field-picker\"][data-md-datepicker-toggle]')))->toBe('the target'); @@ -580,6 +584,11 @@ it('opens the range picker full screen below 600px and grows its month list both $page->click(day('trip-field', '2026-09-20')) ->click(day('trip-field', '2026-09-24')) + // What Save is about to commit, read from the picker before it is pressed: a press that + // landed on another day — the month list grows both ways as it scrolls, so a cell can move + // under one — otherwise shows up only as the value never reaching the server, which says + // nothing about which half went wrong. + ->assertScript("JSON.stringify({$scope}.draft) === '{\"start\":\"2026-09-20\",\"end\":\"2026-09-24\"}'") ->click('#trip-field-picker [data-md-datepicker-save]') ->assertSeeIn('#trip', '{"start":"2026-09-20","end":"2026-09-24"}') ->assertScript("! {$picker}.open"); @@ -590,7 +599,9 @@ it('closes the full-screen range picker from its app bar close button without ke $page = dateProbe() ->resize(390, 800) - ->assertScript('window.innerWidth === 390 && document.documentElement.getAnimations({ subtree: true }).length === 0'); + // Settled before the press, as the test above says at length. + ->assertScript('window.innerWidth === 390') + ->assertScript(settled('document.documentElement', subtree: true)); expect($page->script(hitTarget('[aria-controls=\"trip-field-picker\"][data-md-datepicker-toggle]')))->toBe('the target'); diff --git a/tests/Browser/NavigationTest.php b/tests/Browser/NavigationTest.php index 1047fb00..0c4a3db0 100644 --- a/tests/Browser/NavigationTest.php +++ b/tests/Browser/NavigationTest.php @@ -647,37 +647,15 @@ it('reads expanded while a modal rail is open over the page, and collapsed once }); /** - * Closes a rail with the given script and reports whether, sampling every few ms in the page - * itself, it was ever caught part-way: the whole close in one round trip, since a separate - * `script` and `assertScript` apiece already take as long as the 200ms exit (see ContainmentTest's - * scrim fades). Called after slowMotion(), so the panel's own close — on the emphasized-accelerate - * easing, which stays close to its start for a good part of its run — still needs a couple of - * seconds of that stretched exit before its position has moved the couple of pixels `$partWay` - * looks for; 2000ms (400 × 5ms) covers that with room to spare, short of the full exit ending. + * Closes a rail with the given script and reports whether, sampling once a frame in the page + * itself, it was ever caught part-way (caughtMidExit() in tests/Pest.php). Called after + * slowMotion(), so the panel's own close — on the emphasized-accelerate easing, which stays close + * to its start for a good part of its run — has moved the couple of pixels `$partWay` looks for + * within a handful of the stretched exit's frames. */ function caughtMidClose(mixed $page, string $close, string $partWay): bool { - return $page->script(<< { - {$close} - - for (let i = 0; i < 400; i++) { - if ({$partWay}) return true - await new Promise((resolve) => setTimeout(resolve, 5)) - } - return false - })() - JS) === true; -} - -/** - * Opened and done opening: no transition left on the element. Firefox creates a transition on its - * next refresh tick, so straight after the change its computed style already reads the end value - * while the entry has not begun, and a close then would start from where the entry did. - */ -function settled(string $element): string -{ - return "{$element}.getAnimations().length === 0"; + return $page->script(caughtMidExit($close, $partWay)) === true; } /** Still drawn, and strictly between resting at the window's edge and gone past it. */ @@ -690,7 +668,8 @@ it('slides the compact rail out on close, rather than making it vanish', functio $page = shellPage(599, 860) ->click('@shell-menu') ->assertScript(RAIL.".hasAttribute('data-md-open')") - ->assertScript(settled(RAIL_PANEL).' && Math.round('.RAIL_PANEL.'.getBoundingClientRect().left) === 0'); + ->assertScript(settled(RAIL_PANEL)) + ->assertScript('Math.round('.RAIL_PANEL.'.getBoundingClientRect().left) === 0'); slowMotion($page); @@ -710,7 +689,8 @@ it('slides a rail that hides when collapsed out on close, rather than making it ->assertScript("Math.round({$rail}.getBoundingClientRect().width) === 0") ->click('#open-hiding-rail') ->assertScript("{$rail}.hasAttribute('data-md-open')") - ->assertScript(settled($panel)." && Math.round({$panel}.getBoundingClientRect().left) === 0 && Math.round({$panel}.getBoundingClientRect().width) === 256"); + ->assertScript(settled($panel)) + ->assertScript("Math.round({$panel}.getBoundingClientRect().left) === 0 && Math.round({$panel}.getBoundingClientRect().width) === 256"); slowMotion($page); @@ -727,7 +707,8 @@ it('fades a modal rail\'s scrim out on close while the rail stands collapsed in $page = railValueProbe('modal', 1000) ->click('#open-rail') ->assertScript(RAIL.".hasAttribute('data-md-open')") - ->assertScript(settled($scrim)." && getComputedStyle({$scrim}).opacity === '1'"); + ->assertScript(settled($scrim)) + ->assertScript("getComputedStyle({$scrim}).opacity === '1'"); $fading = "(getComputedStyle({$scrim}).display !== 'none' && parseFloat(getComputedStyle({$scrim}).opacity) > 0.02 && parseFloat(getComputedStyle({$scrim}).opacity) < 0.98)"; diff --git a/tests/Browser/PickingTest.php b/tests/Browser/PickingTest.php index 1cea0c26..2aa4d83b 100644 --- a/tests/Browser/PickingTest.php +++ b/tests/Browser/PickingTest.php @@ -223,20 +223,20 @@ it('says so when nothing matches, and closes on a press outside or a chosen resu */ function searchCloseSample(string $element, string $expression): string { - return << { - const root = document.querySelector('[data-md-search]:has(#find)') - root.querySelector('[data-md-search-scrim]').dispatchEvent(new PointerEvent('pointerdown', { bubbles: true })) + return caughtMidExit( + <<<'JS' + const root = document.querySelector('[data-md-search]:has(#find)') + root.querySelector('[data-md-search-scrim]').dispatchEvent(new PointerEvent('pointerdown', { bubbles: true })) + JS, + << { + const element = root.querySelector('{$element}') + const value = {$expression} - for (let i = 0; i < 80; i++) { - const element = root.querySelector('{$element}') - const value = {$expression} - if (getComputedStyle(element).display !== 'none' && value > 0.02 && value < 0.98) return true - await new Promise((resolve) => setTimeout(resolve, 5)) - } - return false + return getComputedStyle(element).display !== 'none' && value > 0.02 && value < 0.98 })() - JS; + JS, + ); } it('fades the docked search\'s scrim out on close, rather than making it vanish', function () { @@ -245,8 +245,12 @@ it('fades the docked search\'s scrim out on close, rather than making it vanish' $page = pickProbe() ->click('#find') // Displayed as well as opaque: until the first frame of its entry the scrim is still - // `display: none` at the open state's full opacity. - ->assertScript("(({ display, opacity }) => display !== 'none' && opacity === '1')(getComputedStyle({$root}.querySelector('[data-md-search-scrim]')))"); + // `display: none` at the open state's full opacity. And settled, not still fading in: an + // engine reads a transition's end value until its next refresh tick, so an opacity of 1 + // alone does not say the entry has run — and a close sampled from there has nothing left + // to fade. + ->assertScript("(({ display, opacity }) => display !== 'none' && opacity === '1')(getComputedStyle({$root}.querySelector('[data-md-search-scrim]')))") + ->assertScript(settled("{$root}.querySelector('[data-md-search-scrim]')")); slowMotion($page); @@ -260,7 +264,9 @@ it('keeps the docked search above the page while its view closes', function () { $page = pickProbe() ->click('#find') - ->assertScript("getComputedStyle({$view}).display !== 'none' && getComputedStyle({$view}).opacity === '1'"); + ->assertScript("getComputedStyle({$view}).display !== 'none' && getComputedStyle({$view}).opacity === '1'") + // Settled, not still fading in: see the scrim's fade above. + ->assertScript(settled($view)); slowMotion($page); @@ -334,26 +340,27 @@ it('expands the search icon into the full-screen view, and hands focus back to t * Closes a full-screen search from its back arrow and samples, in the page, for a moment in its * exit where the layout is still full screen — the root marked, the header bar fixed at the top — * with both the bar and the view part-way through their fade, rather than the bar gone back to - * its resting form (or to nothing, behind the icon) on the first frame. + * its resting form (or to nothing, behind the icon) on the first frame. Call it after + * slowMotion(): a loaded runner paints only a handful of frames a second, and a real 350ms exit + * can pass between two of them with nothing in between to catch. */ function fullScreenCloseSample(string $input): string { - return << { - const root = document.querySelector('[data-md-search]:has(#{$input})') - root.querySelector('[data-md-search-back]').click() + return caughtMidExit( + << { + const bar = getComputedStyle(root.querySelector('[data-md-search-bar]')) + const view = getComputedStyle(root.querySelector('[data-md-search-view]')) + const fading = (style) => style.display !== 'none' && parseFloat(style.opacity) > 0.02 && parseFloat(style.opacity) < 0.98 - for (let i = 0; i < 80; i++) { - const bar = getComputedStyle(root.querySelector('[data-md-search-bar]')) - const view = getComputedStyle(root.querySelector('[data-md-search-view]')) - const fading = (style) => style.display !== 'none' && parseFloat(style.opacity) > 0.02 && parseFloat(style.opacity) < 0.98 - - if (! root.hasAttribute('data-md-open') && root.hasAttribute('data-md-full-screen') && bar.position === 'fixed' && fading(bar) && fading(view)) return true - await new Promise((resolve) => setTimeout(resolve, 5)) - } - return false + return ! root.hasAttribute('data-md-open') && root.hasAttribute('data-md-full-screen') && bar.position === 'fixed' && fading(bar) && fading(view) })() - JS; + JS, + ); } it('closes the full-screen view and its bar together, back into the search icon', function () { @@ -365,7 +372,9 @@ it('closes the full-screen view and its bar together, back into the search icon' ->assertScript("{$root}.hasAttribute('data-md-full-screen')") // Its entry run: Firefox reads a transition's end value until its next refresh tick, so an // opacity of 1 alone does not say the view has finished fading in. - ->assertScript("{$root}.getAnimations({ subtree: true }).length === 0"); + ->assertScript(settled($root, subtree: true)); + + slowMotion($page); expect($page->script(fullScreenCloseSample('find-icon')))->toBeTrue(); @@ -384,7 +393,9 @@ it('closes a compact window\'s full-screen view and its bar together, back into $page->click('#find') ->assertScript("{$root}.hasAttribute('data-md-full-screen')") - ->assertScript("{$root}.getAnimations({ subtree: true }).length === 0"); + ->assertScript(settled($root, subtree: true)); + + slowMotion($page); expect($page->script(fullScreenCloseSample('find')))->toBeTrue(); diff --git a/tests/Pest.php b/tests/Pest.php index a88208a8..32b95073 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -41,6 +41,11 @@ function layoutRoot(string $html, ?string $tag = null): array * starts or after it has already finished on a loaded runner. Chain it, after the open has * settled, immediately before the action that triggers the exit being sampled. * + * Three seconds, not one: a loaded runner paints only a handful of frames a second, and a one-second + * exit can begin and end between two of them with no frame in between showing it part-way. It costs + * nothing in runtime — what made those tests take 45 seconds apiece was a script sleeping through + * the exit in the page, past the plugin's budget for one attempt (onceInPage()), not the stretch. + * * Use this only in a test asserting *that* something animates — a part-way opacity, offset or * transform. Never in a test asserting a duration value, which this would make wrong. */ @@ -67,6 +72,90 @@ function slowMotion(mixed $page, string $duration = '3s'): mixed return $page; } +/** + * A JS expression answering whether `$element` has finished whatever it animates — chained as an + * assertScript() after an open, before a test stretches motion and samples the exit, or before it + * presses something whose box is still moving. `$subtree` reads its descendants' animations too. + * + * Two animation frames first, because an empty `getAnimations()` means two different things: an + * entry that has run, and one the engine has not created yet. Until the first style update after + * `x-show` reveals an element its `@starting-style` entry does not exist, and the computed style + * already reads the open value, so a bare "no animations, opacity 1" passes on a page that has + * drawn nothing since the open — and the exit sampled from there turns out to be an entry barely + * begun, with nothing left to catch. A runner painting a handful of frames a second sits in that + * window for hundreds of milliseconds at a time. + */ +function settled(string $element, bool $subtree = false): string +{ + $options = $subtree ? '{ subtree: true }' : ''; + + return << { + await new Promise((resolve) => requestAnimationFrame(resolve)) + await new Promise((resolve) => requestAnimationFrame(resolve)) + + return {$element}.getAnimations({$options}).length === 0 + })() + JS; +} + +/** + * Runs `$expression` — an async IIFE — in the page once, however often the plugin asks for it, and + * answers with what that one run returned. + * + * The plugin gives each script() and assertScript() attempt 1000ms and then runs the whole + * expression again, for 45 seconds, before one last attempt with the full timeout (AwaitableWebpage, + * Execution::waitForExpectation). Anything that takes longer than a second in the page therefore + * runs some fifty times and only succeeds on that last attempt, which costs 45 seconds a test — + * and where the expression presses something, every run after the first presses it again, on a page + * the first run has already left somewhere else, so what comes back describes none of them. + */ +function onceInPage(string $expression): string +{ + $memo = '__onceInPage'.crc32($expression); + + return "(() => (window.{$memo} ??= {$expression}))()"; +} + +/** + * A JS expression that runs `$trigger` in the page and then samples `$condition` there once per + * animation frame, answering whether it ever held: the whole exit and its sample in one round trip, + * since a separate script() and assertScript() apiece already cost as much as a real exit. + * + * On animation frames rather than a timer, which is what the sampling loops used to do: a loaded + * engine — WebKitGTK on the runner — stops painting while a tight `setTimeout` loop holds the main + * thread, so the CSS transition never advances and every sample reads the opening value, then + * `display: none`. The exit was reported as a vanishing when nothing had gone wrong but the frames. + * Asking for a frame both paces the loop to the renderer and makes it run, so each sample is a + * frame someone would have seen. + * + * The budget covers most of a slowMotion() exit, because a loaded runner can leave a second between + * two frames of a heavy page and the one frame that shows the exit half-way may be the third. + * + * Sampled once through onceInPage(), because triggering the exit a second time on a page whose + * first one is long finished would report a miss whatever the page did the first time. + */ +function caughtMidExit(string $trigger, string $condition, int $budget = 2500): string +{ + return onceInPage(<< { + {$trigger} + + const deadline = performance.now() + {$budget} + + while (performance.now() < deadline) { + if ({$condition}) { + return true + } + + await new Promise((resolve) => requestAnimationFrame(resolve)) + } + + return false + })() + JS); +} + /** * The page is done loading, and Alpine and/or Livewire (whichever the page renders) have booted — * chained after visit(), before a Browser test probe reads anything either one wires up. Most