- Notifications
You must be signed in to change notification settings - Fork4.9k
chore(bidi): add support for context locators#38129
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
| private_onDomContentLoaded(params:bidi.BrowsingContext.NavigationInfo){ | ||
| privateasync_onDomContentLoaded(params:bidi.BrowsingContext.NavigationInfo){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Event listeners should be synchronous, we don't want to mess up the event order due to random awaits.
The frame name usually doesn't change, so we could query it on frame attach and if it arrives before the first navigation it will automatically propagate to the clients. This would be racy in some cases, but doesn't require async event handlers. We now have locators (and frame locators) that allow to select iframe elements and don't depend on this method, this could be a fallback for the folks that need to find a frame.
I don't think we should make all these handlers async just for the frame.name(). I'd rather keep the property best effort. If we want a proper fix, we'd need to make the name available synchronously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have updated the PR accordingly,frame.name() andpage.frame({ name }) are now best effort.
I have updated the tests to use different ways to get a frame's name or find a frame by name and disabled those tests for BiDi that explicitly want to test the behavior offrame.name() andpage.frame({ name }).
| it('should report frame.name()',async({ page, server})=>{ | ||
| it('should report frame.name()',async({ page, server, browserName, channel})=>{ | ||
| it.skip(browserName==='firefox'&&channel?.startsWith('moz-firefox'),'frame.name() is racy with BiDi'); | ||
| it.skip(channel?.startsWith('bidi-chrom'),'frame.name() is racy with BiDi'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FWIW, this test could be fixed by using
expect((await (await page.frames()[1].frameElement()).getAttribute('id'))).toBe('theFrameId');expect((await (await page.frames()[2].frameElement()).getAttribute('name'))).toBe('theFrameName');There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As the test's title suggests it validatesframe.name() behavior, so the code should invokename() at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, that's why I didn't include this in the PR. I was just mentioning it to show what the alternative toframe.name() for BiDi looks like.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbenl commentedNov 10, 2025
CI test results compared to the last scheduled test run:https://firefox-dev.tools/playwright-bidi-dashboard/diff.html?browser=firefox&pr=38129 (all new failures seem to be intermittents, even those that are not detected as such). |
Uh oh!
There was an error while loading.Please reload this page.
| this.clearWebSockets(frame); | ||
| frame._url=url; | ||
| frame._name=name; | ||
| frame._name=name||frame._name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should go into Bidi-specific code.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| it('should report frame.name()',async({ page, server})=>{ | ||
| it('should report frame.name()',async({ page, server, browserName, channel})=>{ | ||
| it.skip(browserName==='firefox'&&channel?.startsWith('moz-firefox'),'frame.name() is racy with BiDi'); | ||
| it.skip(channel?.startsWith('bidi-chrom'),'frame.name() is racy with BiDi'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As the test's title suggests it validatesframe.name() behavior, so the code should invokename() at some point.
Uh oh!
There was an error while loading.Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
yury-s commentedNov 12, 2025
Nice! Once the merge conflict is resolved I can merge it. |
Test results for "MCP"2 flaky2430 passed, 116 skipped Mergeworkflow run. |
Test results for "tests 1"2 failed 4 flaky40255 passed, 787 skipped Mergeworkflow run. |
Uh oh!
There was an error while loading.Please reload this page.
This PR uses BiDi context locators to implement
Frame.frameElement()and to fetch a frame'sname(orid) attribute.For the latter I ran into two timing issues:
FrameManager.frameCommittedNewDocumentNavigation()needs to be called synchronously when the navigation is committed because it removes the old child frames, so if we wait for the frame's name to be fetched before calling it, we may remove child frames that were added after the navigation. I fixed this by calling it synchronously, passing a Promise for the name to it and awaiting that Promise only after deleting the child frames.domcontentloadedandloadevents should only be fired after all child frame names have been fetched, otherwise a test may request a child frame by name before that name has been fetched.The PRfixes#38098 and the following tests in Firefox:
tests/library/hit-target.spec.ts:tests/page/elementhandle-bounding-box.spec.ts:tests/page/frame-frame-element.spec.ts:tests/page/frame-hierarchy.spec.ts:"should handle nested frames""should report frame.name()"