Skip to content

Conversation

@whimboo
Copy link
Contributor

@whimboo whimboo commented Mar 19, 2025

We got https://bugzilla.mozilla.org/show_bug.cgi?id=85686 in Firefox fixed for version 138 which is currently in Nightly. That means that we no longer need to special-case the handling for getSelection() with Firefox.

I assume this PR needs to wait with getting merged until Firefox 138 is going to be released?

@sefeng211
Copy link

It's Nightly only feature for now, I plan to give it some time before enabling it. So we should only remove the special-case handling after it's enabled in Release

@whimboo whimboo marked this pull request as draft March 19, 2025 13:11
@whimboo
Copy link
Contributor Author

whimboo commented Mar 19, 2025

Oh, good point. Lets move this PR to draft for now.

@whimboo
Copy link
Contributor Author

whimboo commented Mar 19, 2025

Enabling by default for all Firefox builds is covered by https://bugzilla.mozilla.org/show_bug.cgi?id=1954979.

@github-actions
Copy link
Contributor

Test results for "tests 1"

4 failed
❌ [firefox-page] › tests/page/elementhandle-select-text.spec.ts:20:3 › should select textarea @firefox-ubuntu-22.04-node18
❌ [firefox-page] › tests/page/elementhandle-select-text.spec.ts:33:3 › should select input @firefox-ubuntu-22.04-node18
❌ [firefox-page] › tests/page/locator-misc-2.spec.ts:76:3 › should select textarea @firefox-ubuntu-22.04-node18
❌ [firefox-page] › tests/page/retarget.spec.ts:212:3 › input value retargeting @firefox-ubuntu-22.04-node18

4 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:773:1 › should handle src=blob @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38780 passed, 808 skipped
✔️✔️✔️

Merge workflow run.

@yury-s
Copy link
Member

yury-s commented Apr 4, 2025

@whimboo what is the status of this? The Firefox bug has been closed, do we just wait for the next FF roll?

FYI, I'm also enabling the tests in WebKit: #35498.

@whimboo
Copy link
Contributor Author

whimboo commented Apr 7, 2025

@whimboo what is the status of this? The Firefox bug has been closed, do we just wait for the next FF roll?

The fix will be in Firefox 139 - where it will be enabled by default. It will ride the trains and will be released on May 27th. If you want to get rid of this code already for 138 which is on beta now, you could temporarily use the dom.selection.mimic_chrome_tostring.enabled preference and set it to true. It's release will be on April 29th.

But I think that this change can wait for the 139 release given that it was broken for such a long time.

@yury-s
Copy link
Member

yury-s commented Apr 7, 2025

Ok, let's wait until it is released.

@whimboo
Copy link
Contributor Author

whimboo commented May 21, 2025

I updated the PR to fix the actual merge conflicts in the tests. Do you already have an ETA for when your custom Firefox 139 will be available?

@github-actions

This comment has been minimized.

@yury-s
Copy link
Member

yury-s commented May 22, 2025

Do you already have an ETA for when your custom Firefox 139 will be available?

We usually try to include current release of Firefox, so it we should roll 139 soon after it becomes stable. You can track our rolls in https://github.com/microsoft/playwright-browsers/blob/main/browser_patches/firefox/BUILD_NUMBER

@whimboo whimboo marked this pull request as ready for review June 6, 2025 08:54
@whimboo
Copy link
Contributor Author

whimboo commented Jun 6, 2025

@yury-s can you please review? Thanks.

@yury-s
Copy link
Member

yury-s commented Jun 6, 2025

The change looks good to me, let's rebase an make sure the tests pass.

@github-actions

This comment has been minimized.

@mxschmitt mxschmitt merged commit a7df837 into microsoft:main Jun 7, 2025
29 checks passed
@mxschmitt
Copy link
Contributor

Thank you!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2025

Test results for "tests 1"

5 flaky ⚠️ [chromium-library] › library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @chromium-ubuntu-22.04-node22
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:986:7 › cli codegen › should not throw csp directive violation errors @firefox-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-event-popup.spec.ts:28:3 › should work with window features @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

39353 passed, 820 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants