Repository navigation
fix(site): copy the playground link in Safari, and keep the Apply all row in place - #249
Merged
Merged
Conversation
… row in place Copy link starts the clipboard write in the click, with the link as a promise in a ClipboardItem, as Copy page does: Safari refused the writeText that ran after the link was worked out. When the write fails, the status says the link is in the address bar. While edited code is checked again, the "Apply N fixes" row keeps the last result's counts with its button off, as the findings below do, instead of going and coming back. Opening a #code= link at 1440 px shifted the findings by 41 px twice (CLS 0.0399, now 0.0029). Tests: drop the touch-target test small-screens.test.ts repeated, and check ask.tsx and site-menu.tsx in polish.test.ts. ROADMAP: #200 is merged, and "Open in playground" on rule pages is next.
#201 shipped in 0.9.1, #202 in 0.10.0 and #199 in 0.10.1, as the changelog shows; #200, #203 and #204 are website and docs changes, live since they merged. Main is at 0.10.1 with no pending changesets, so nothing is merged and waiting for a release. In review now lists the open pull requests #241 and #242.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The polish left from the review of #200. Site only, no changeset. Roadmap item: os-200-polish.
What was wrong
copyLinkwaited forlinkHash()and then callednavigator.clipboard.writeText. Safari refuses a clipboard write that starts after the click handler has awaited something. A failed write was also silent: the rejection handler was() => undefined.#code=link was being checked, the "Apply N fixes" row went away (fixablewas[]while stale), then came back with the result. At 1440 px the row sits on its own line under the heading, so the findings list moved from y 509 to 468 and back. That is two layout shifts of 0.0199 each, CLS 0.0399.site/test/small-screens.test.tsrepeated the one insite/test/polish.test.ts.What changed
site/components/playground.tsxcopyText(text: Promise<string>)starts the write as part of the click. The link goes in as a promise inside aClipboardItem, the same waycopyMarkdowninpage-actions.tsxdoes it. Where there is noClipboardItem, it falls back towriteText.copyLinkbuilds the URL inside that promise and updates the address bar there, as before.ApplyAllcomponent. While the edited code is checked again, it keeps the last result's counts, faded, with its button disabled. This is what each finding's own "Apply fix" button below already does. It no longer goes away and comes back. Once the new result is in, it shows the new counts, or goes away if no finding has a fix.site/test/small-screens.test.ts: removed the duplicatetouch targetsblock and the imports only it used.site/test/polish.test.ts: addedcomponents/ask.tsxto the files checked for thetapclass. I also addedcomponents/site-menu.tsx, which only the removed test checked, so the menu button keeps its check.ROADMAP.md, the two status sections only. I checked each line against GitHub's merge dates,gh release listandCHANGELOG.md:/rules/<id>pages. It needs the playground'slinkHashmoved intosite/lib/. Nothing else in the file changed.A side effect of keeping the row: after "Apply all", the pressed button stays visible but disabled until the check returns, then goes away. Before, it went away at once. Focus still moves to the summary, as before. The findings list changes at that same moment anyway, because the fixed findings drop out of it.
How it was tested
New file
site/test/playground-share.test.ts, 4 tests. All 4 fail on main, where neithercopyTextnorApplyAllexists, and pass on this branch:ClipboardItemit falls back towriteText. It rejects when the browser refuses the write, and when there is no clipboard at all.CLS while opening a
#code=link (the first draft plus one comment line). I measured it with a PlaywrightPerformanceObserver(layout-shift, inputs excluded) in Chromium at 1440 x 900, againstnext starton a local production build, 5 runs each:At 375 px, the branch measured 0.0000 in 5 runs. What is left at 1440 px (two shifts of 0.0014) is not the row. The status in the editor bar changes width (the error and warning count, then "Checking…"), so it moves sideways, and the error marks in the code are redrawn. The 0.0001 is the page heading moving 3 px. WebKit does not report
layout-shiftentries, so there are no WebKit CLS numbers.Copy link, in Playwright Chromium (clipboard read back) and WebKit (live region read). Each was tested with the link ready and with a click straight after an edit, with link compression slowed by 300 ms so the click lands first. Both cases copy the right URL on this branch. With the clipboard write made to fail, main's live region stays empty and this branch announces the failure, in both engines. Playwright's WebKit does not enforce Safari's rule about writes after an await, since main copies there too. So the Safari fix rests on the ClipboardItem pattern
page-actions.tsxalready uses, and on the test that the write starts during the click.I also checked in Chromium at 1440 px and WebKit at 375 px: while typing, the row stays disabled and faded; after the check, it is enabled again. After Apply all, focus moves to the summary and the row goes away once no fix is left.
Gate:
pnpm install --frozen-lockfile --prefer-offline: okpnpm format:check,pnpm lint,pnpm typecheck: cleanpnpm test: 102 files and 1290 tests passed, 1 file and 3 tests skippedpnpm build: ok, publint found no issuespnpm --filter site generate,test,typecheck,lint,build: ok. The site tests ran 21 files and 288 tests.Not in this PR