Skip to content

Conversation

@hbenl
Copy link
Collaborator

@hbenl hbenl commented Nov 6, 2025

This PR uses BiDi context locators to implement Frame.frameElement() and to fetch a frame's name (or id) 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.
  • the domcontentloaded and load events 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 the following tests in Firefox:

  • tests/library/hit-target.spec.ts:
    • "should click into frame inside closed shadow root"
  • tests/page/elementhandle-bounding-box.spec.ts:
    • "should handle nested frames"
  • tests/page/frame-frame-element.spec.ts:
    • "should work inside closed shadow root"
    • "should work inside declarative shadow root"
  • tests/page/frame-hierarchy.spec.ts:
    • "should handle nested frames"
    • "should report frame.name()"

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@dgozman dgozman requested a review from yury-s November 6, 2025 22:33
@hbenl hbenl marked this pull request as draft November 7, 2025 13:57
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2025

Test results for "MCP"

2 failed
❌ [firefox] › mcp/test-debug.spec.ts:125 › test_debug (multiple pages and custom error) @mcp-ubuntu-latest
❌ [firefox] › mcp/test-debug.spec.ts:166 › test_debug (pause/snapshot/resume) @mcp-ubuntu-latest

2586 passed, 116 skipped


Merge workflow run.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2025

Test results for "tests 1"

3 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-event-request.spec.ts:182 › should return response body when Cross-Origin-Opener-Policy is set `@firefox-ubuntu-22.04-node18`
⚠️ [chromium] › headerView.spec.tsx:49 › should toggle filters `@web-components-html-reporter`

40319 passed, 789 skipped


Merge workflow run.

@hbenl hbenl marked this pull request as ready for review November 7, 2025 17:07
}

private _onDomContentLoaded(params: bidi.BrowsingContext.NavigationInfo) {
private async _onDomContentLoaded(params: bidi.BrowsingContext.NavigationInfo) {
Copy link
Member

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.

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.

2 participants