chore(bidi): add support for context locators #38129
Conversation
hbenl
force-pushed
the
context-locators
branch
from
60e751d to
58ac4ba
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbenl
force-pushed
the
context-locators
branch
from
58ac4ba to
7184989
Compare
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) { | ||
| private async _onDomContentLoaded(params: bidi.BrowsingContext.NavigationInfo) { |
There was a problem hiding this comment.
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.
I have updated the PR accordingly, frame.name() and page.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 of frame.name() and page.frame({ name }).
hbenl
force-pushed
the
context-locators
branch
from
7184989 to
7c73c15
Compare
| 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.
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.
As the test's title suggests it validates frame.name() behavior, so the code should invoke name() at some point.
There was a problem hiding this comment.
Yes, that's why I didn't include this in the PR. I was just mentioning it to show what the alternative to frame.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.
|
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). |
| this.clearWebSockets(frame); | ||
| frame._url = url; | ||
| frame._name = name; | ||
| frame._name = name || frame._name; |
There was a problem hiding this comment.
This should go into Bidi-specific code.
| 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.
As the test's title suggests it validates frame.name() behavior, so the code should invoke name() at some point.
hbenl
force-pushed
the
context-locators
branch
from
7c73c15 to
35fbd84
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Nice! Once the merge conflict is resolved I can merge it. |
hbenl
force-pushed
the
context-locators
branch
from
35fbd84 to
581102e
Compare
Test results for "MCP"2 flaky2430 passed, 116 skipped Merge workflow run. |
Test results for "tests 1"2 failed 4 flaky40255 passed, 787 skipped Merge workflow run. |
2 participants
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 PR fixes #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()"