[fix] unselect before navigation #2755
Conversation
🦋 Changeset detectedLatest commit: 356b436 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| /** @type {import('test').TestMaker} */ | ||
| export default function (test) { | ||
| test('reset selection', '/selection/a', async ({ page, clicknav }) => { | ||
| await page.waitForTimeout(50); |
There was a problem hiding this comment.
Is there a reason to wait for timeout? We have tried to avoid this if possible. If it's really needed, we should add a comment explaining it.
| 1 | ||
| ); | ||
| await clicknav('[href="/selection/b"]'); | ||
| await page.waitForTimeout(50); |
There was a problem hiding this comment.
Test was inconsistent without that timeout, on my setup at least. Maybe someone can run tests to confirm.
There was a problem hiding this comment.
Can't reproduce inconsistencies so waitForTimeout calls was removed. By the way, is there a docker file or something for contributors, to build/test project?
There was a problem hiding this comment.
By the way, is there a docker file or something for contributors, to build/test project?
There isn't one that I know of, but I don't have much issues with local test inconsistencies in the past. Playwright ships its own chromium browser IIRC so that should eliminate most of the cross-machine quirks.
bluwy
changed the title
… the renderer.js:update method. removed waitForTimeout calls from test.
| @@ -275,6 +275,8 @@ export class Renderer { | |||
| this._init(navigation_result); | |||
| } | |||
|
|
|||
| getSelection()?.removeAllRanges(); | |||
|
|
|||
There was a problem hiding this comment.
not exactly related to your change, but if (!opts) shouldn't the document.body.focus() line still be called? I don't quite get what that check is doing and wonder if it should be removed? maybe i'm just sleep deprived enough that I'm making a dumb mistake in reading this code...
There was a problem hiding this comment.
I'm actually quite confused of the part inside if (!opts) too. When I refactored that, I tried to keep it as before. That means document.body.focus() and scrolling only happens if opts exist, I thought it was intentional.
If it wasn't, maybe we should refactor that again, and always do focusing and scrolling regardless of opts. We can do that in a separate PR though.
|
excellent, thank you! |
| 1 | ||
| ); | ||
| await clicknav('[href="/selection/b"]'); | ||
| assert.equal( |
There was a problem hiding this comment.
i'm worried this might introduce flakiness, which is why you sometimes needed the timeout. I wonder if you need to wait until the selection is present instead of simply checking that it is
|
This is probably causing #2853. |
fixes #1054
Before submitting the PR, please make sure you do the following
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpx changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0