Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[📖 Docs]: switchFrame by index #150

Open
2 tasks done
htho opened this issue Jan 9, 2025 · 19 comments
Open
2 tasks done

[📖 Docs]: switchFrame by index #150

htho opened this issue Jan 9, 2025 · 19 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@htho
Copy link

htho commented Jan 9, 2025

Pre-check

  • I know I can edit the docs but prefer to file this issue

Describe the improvement

Unclear documentation, Missing documentation, Other

Description of the improvement / report

Hi,

I automatically transform steps recorded by Katalon Recorder to Gherkin.

I have problems with the selectFrame command, which uses the index of the iframe.
I could just use browser.switchToFrame(index); and do something else, but it's deprecated and I am not sure if it works anymore.

I think the documentation should include a drop-in replacement for switchToFrame (by index) using selectFrame.

I came up with some ideas, but I am still learning, so I was not able to get them to work:

const index = 1; // a parsed step parameter

return browser.switchFrame(() => Array.from(window.parent.frames).indexOf(window) === index)

But it fails with: Error: Could not find the desired frame

const index = 1; // a parsed step parameter

const frame = $(function(this: Window) { 
	return this[index]?.frameElement as HTMLElement ?? undefined;
});
return browser.switchFrame(frame)

But it fails with: WebdriverBidiExeception: ReferenceError: index is not defined

This is probably due to the fact, that I don't know how to pass index to the function, which is executed in the browser.

Dispaired as I was, I just ran

const index = 1; // a parsed step parameter

return browser.switchFrame(index); // typescript does not like a number here

Which yields: Error: Invalid type for context parameter: number, expected one of number, string or null. Check out our docs: https://webdriver.io/docs/api/browser/switchToFrame.html

This is interesting: The error message says number is okay, although it isn't and it points at the outdated documentation of switchToFrame.

I think this error message should be fixed.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@christian-bromann
Copy link
Member

Based on the switchFrame docs using an index to refer to a browsing context is not supported. There is no "order" for contexts, hence using an index would be generally a bad idea. There are various ways to can select a context:

  • by providing a url
  • by providing a context id directly (fetch them via const tree = await this.browsingContextGetTree({}))
  • by providing a WebdriverIO element
  • by executing a function in the browser that runs in all available contexts and allows you to check e.g. for an element you would expect in this context

This is interesting: The error message says number is okay, although it isn't and it points at the outdated documentation of switchToFrame.

Thanks I will fix this.

I will go ahead and close this, happy to answer further questions!

christian-bromann referenced this issue in webdriverio/webdriverio Jan 9, 2025
* fix(webdriverio): consolidate session manager

* remove log

* clean up event listener

* only remove listeners from given context manager

* fix context manager

* fix

* fix(webdriverio): fix link in error message - refs #14043
@htho
Copy link
Author

htho commented Jan 9, 2025

Hi, I agree: index is a very bad way to select a frame.

But even the Chrome DevTools recorder uses it. (I wonder if/how the webdriverio plugin for the recorder handles the frame.)

So IMO there should be a (documented) way to do it.

I found out that my 1st approach fails for the same reason my 2nd approach fails: the variable is not passed to the browser. I'd really like to learn how to pass variables to functions that are executed in the browser.

@htho
Copy link
Author

htho commented Jan 9, 2025

About the Change in the error message: Yes, the link is correct now, but the message will still complain that I passed a number and then tell me that number is among the accepted types.

@christian-bromann
Copy link
Member

So IMO there should be a (documented) way to do it.

What is missing in the docs we have?

I'd really like to learn how to pass variables to functions that are executed in the browser.

I am not sure if that makes sense as right now, we run the script in every context which allows plenty of opportunity to verify whether one would be in the right context. Can you explain more about your use case? I don't think passing in an index is a good approach to verify.

Is non of the provided options to select a frame an option for you?

@htho
Copy link
Author

htho commented Jan 9, 2025

Given someone uses Chromes recorder to record a click within an iframe
And someone uses the webdriverio-chromerecorder plugin to export the recording.
Then the exported code will not work, because it will not switch the context.

The frame is indicated by the frame property in the recorded step. The frame property is a number.

If we want to be compatible with the Chrome recorder, there must be a way to switch the context based in the information we have. And the only information we have is a number.

Given that (many) other automation solutions support this, I wonder if we can just ignore the fact that this is a common way to do it.

@christian-bromann
Copy link
Member

Given that (many) other automation solutions support this

Support what? Switching to a frame by index? We could technically support it by e.g. just switch to the frame that matches the index of the list we get from Chromedriver but this seems not to be a good approach overall and we should encourage using better ways.

If we want to be compatible with the Chrome recorder,

I think we should make a change in the Chrome Recorder then and provide a better parameter. I am sure we have all necessary information there to do so. You want to take a stab at it?

@christian-bromann christian-bromann transferred this issue from webdriverio/webdriverio Jan 9, 2025
@christian-bromann
Copy link
Member

Moving the issue into the Chrome recorder repository.

@christian-bromann christian-bromann added bug Something isn't working help wanted Extra attention is needed labels Jan 9, 2025
@htho
Copy link
Author

htho commented Jan 10, 2025

Switching to a frame by index? We could technically support it by e.g. just switch to the frame that matches the index of the list we get from Chromedriver but this seems not to be a good approach overall and we should encourage using better ways.

If there was a documented workaround (like the approaches I initially posted), we would would not encourage it, because the API still does not support it. But the problem would be solvable for those that have to deal with frame indexes they get from somewhere else.

I think we should make a change in the Chrome Recorder then and provide a better parameter. I am sure we have all necessary information there to do so. You want to take a stab at it?

I assume you mean our webdriverio/chrome-recorder plugin?
Because I feel contributing to the actual recorder in chrome is not that simple.

I fear that the information is not available.

Given a DOM like this:

<body>
	<iframe id=A src="iframe.html"></iframe>
	<iframe id=B src="iframe.html"></iframe>
	<iframe id=C src="iframe.html"></iframe>
</body>

And record these steps:

  1. Click in A
  2. Click in B
  3. Click in C
  4. Click in outside any iframe

The resulting recording (which the chrome-recorder plugin transforms) looks like this:

{
    "title": "Recording 10.1.2025 at 07:08:16",
    "steps": [
        {
            "type": "setViewport",
            // omitted
        },
        {
            "type": "navigate",
            // omitted
        },
        {
            "type": "click",
            "target": "main",
            "selectors": [],
            "offsetY": 84,
            "offsetX": 158,
            "frame": [
                0
            ]
        },
        {
            "type": "click",
            "target": "main",
            "selectors": [],
            "offsetY": 82,
            "offsetX": 111,
            "frame": [
                1
            ]
        },
        {
            "type": "click",
            "target": "main",
            "selectors": [],
            "offsetY": 90,
            "offsetX": 94,
            "frame": [
                2
            ]
        },
        {
            "type": "click",
            "target": "main",
            "selectors": [
                [
                    "body"
                ]
            ],
            "offsetY": 132,
            "offsetX": 998
        }
    ]
}

This is the data that the chrome devtools recorder provides to the stringifyStep method
As far as I can tell, the information is just not there.

The way I see it, we should help people to deal with frame indexes, if they have to.

@christian-bromann
Copy link
Member

I assume you mean our webdriverio/chrome-recorder plugin?

Yes, the implementation can be found in this repository.

The resulting recording (which the chrome-recorder plugin transforms) looks like this:

Thanks for pulling up this data structure. I think we can transform the click step with a frame property into the following code:

If the selector is given use it:

await browser.switchFrame($$('frame')[1])
await $('body').click()

otherwise click on the coordinates:

await browser.switchFrame($$('frame')[1])
await browser.action(...)

What do you think?

@htho
Copy link
Author

htho commented Jan 10, 2025

What do you think?

Hi, that won't work, because the index is not the same as the position in the DOM.

  • by index I mean the result of Array.from(window.parent.frames).indexOf(window)
  • by position I mean the result of [...iframe.ownerDocument.querySelectorAll("iframe")].indexOf(iframe);

And to make things worse, iframes can be nested.

I created a demo at https://htho.github.io/wdio-repro-iframes/

<!-- THIS IS AN ABSTRACTION -->
<h1>Frame Demo</h1>
<iframe id=A> <!-- index 0, position 0  -->
    <h2>IFrame A</h2>
    <iframe id=A1><h3>IFrame A1</h2></iframe> <!-- index 0, position 0  -->
    <iframe id=A2><h3>IFrame A2</h2></iframe> <!-- index 1, position 1  -->
</iframe>
<iframe id=B><h2>IFrame B</h2></iframe> <!-- added asynchronously --> <!-- index 2, position 1  -->
<iframe id=C><h2>IFrame C</h2></iframe> <!-- index 1, position 2  -->

IFrame B is added asynchronously (later) therefore its index is 2, but its position (in the dom) is 1.

The chrome recorder creates arrays of frame-indexes, rebuilding the path from the root document through all iframes.
For A2 the path is [0, 1]
So the Plugin should also be able to cope with that.

@christian-bromann
Copy link
Member

So what's the index then referring to? Is it a flattened version from what is returned from browser.browsingContextGetTree(params)?

@htho
Copy link
Author

htho commented Jan 10, 2025

Well, not necessarily flattened.

describe('IFrames', () => {
    it('browsingContextGetTree', async () => {
        await browser.url(`https://htho.github.io/wdio-repro-iframes/`);
        await expect($("#B")).toExist();
        const ctx = await browser.browsingContextGetTree({});
        console.log(JSON.stringify(ctx, null, 4));
    });
});

This is the result.
(I moved the "children" property to the bottom of each object, for readability purposes)

{
  "contexts": [
    {
      "clientWindow": "",
      "context": "E57B9D0D8EA00229BBEA4BEF0AB91ED8",
      "originalOpener": null,
      "parent": null,
      "url": "https://htho.github.io/wdio-repro-iframes/",
      "userContext": "default",
      "children": [
        {
          "clientWindow": "",
          "context": "261E2473B6A018FE231E8154F1F877F7",
          "originalOpener": null,
          "url": "https://htho.github.io/wdio-repro-iframes/iframeA.html",
          "userContext": "default",
          "children": [
            {
              "clientWindow": "",
              "context": "80A29B8E9AD882643AA8D6DDCA3405C3",
              "originalOpener": null,
              "url": "https://htho.github.io/wdio-repro-iframes/iframeA1.html",
              "userContext": "default",
              "children": []
            },
            {
              "clientWindow": "",
              "context": "D825D1A25B21F2DF93347B29ECFDE96D",
              "originalOpener": null,
              "url": "https://htho.github.io/wdio-repro-iframes/iframeA2.html",
              "userContext": "default",
              "children": []
            }
          ]
        },
        {
          "clientWindow": "",
          "context": "5156474223E61A67CBF9368377D93103",
          "originalOpener": null,
          "url": "https://htho.github.io/wdio-repro-iframes/iframeC.html",
          "userContext": "default",
          "children": []
        },
        {
          "clientWindow": "",
          "context": "2CA9F233230996834E80AFF301AC75C2",
          "originalOpener": null,
          "url": "https://htho.github.io/wdio-repro-iframes/iframeB.html",
          "userContext": "default",
          "children": []
        }
      ]
    }
  ]
}

From my observation here, the index of B/C in window.parent.frames fits the index of B/C in this object.

@htho
Copy link
Author

htho commented Jan 10, 2025

Based on browsingContextGetTree I found a solution:

import { expect, browser } from '@wdio/globals'

beforeEach(async () => {
    await browser.url(`https://htho.github.io/wdio-repro-iframes/`);
});

// without this, the next test will load the url in the last selected frame
afterEach(() => browser.switchFrame(null))

describe('switchFrame', () => {
    it('browsingContextGetTree', async () => {
        await expect($("#B")).toExist(); // we don't care for #B in the other tests

        const ctx = await browser.browsingContextGetTree({});
        console.log(JSON.stringify(ctx, null, 4));
        
        console.log([], await getContextByFramePath([]));
        console.log([0], await getContextByFramePath([0]));
        console.log([0, 0], await getContextByFramePath([0, 0]));
        console.log([0, 1], await getContextByFramePath([0, 1]));
        console.log([1], await getContextByFramePath([1]));
        console.log([2], await getContextByFramePath([2]));
        expect(() => getContextByFramePath([0, 2])).rejects.toThrowError("Frame not found [0,2]");
    });

    it('changes the frame by an element reference', async () => {
        await expect($("h1")).toHaveText("Frame Demo");
        await expect($("h2")).not.toExist();
        await expect($("h3")).not.toExist();

        await browser.switchFrame($("#A"));
        await expect($("h1")).not.toExist();
        await expect($("h2")).toHaveText("IFrame A");
        await expect($("h3")).not.toExist();
        
        await browser.switchFrame($("#A2"));
        await expect($("h1")).not.toExist();
        await expect($("h2")).not.toExist();
        await expect($("h3")).toHaveText("IFrame A2");
    });

    it('changes the frame by index path (absolute) using getContextByFramePath', async () => {
        await expect($("h1")).toHaveText("Frame Demo");
        await expect($("h2")).not.toExist();
        await expect($("h3")).not.toExist();

        await browser.switchFrame(await getContextByFramePath([0]));
        await expect($("h1")).not.toExist();
        await expect($("h2")).toHaveText("IFrame A");
        await expect($("h3")).not.toExist();
        
        await browser.switchFrame(await getContextByFramePath([0, 1]));
        await expect($("h1")).not.toExist();
        await expect($("h2")).not.toExist();
        await expect($("h3")).toHaveText("IFrame A2");
    });

    it('it changes the frame by local index (relative) using getContextByFrame', async () => {
        await expect($("h1")).toHaveText("Frame Demo");
        await expect($("h2")).not.toExist();
        await expect($("h3")).not.toExist();

        await browser.switchFrame(await getContextByFrame(0));
        await expect($("h1")).not.toExist();
        await expect($("h2")).toHaveText("IFrame A");
        await expect($("h3")).not.toExist();
        
        await browser.switchFrame(await getContextByFrame(1));
        await expect($("h1")).not.toExist();
        await expect($("h2")).not.toExist();
        await expect($("h3")).toHaveText("IFrame A2");
    });
});

async function getCurrentContext() {
    // based on switchToFrameUsingElement
    // https://github.com/webdriverio/webdriverio/blob/2d653ca7304cd1af6c7519cd9fe3c25edf984609/packages/webdriverio/src/commands/browser/switchFrame.ts#L323
    const win = await browser.execute(
        () => window
    ) as unknown as { context: string };
    return win.context;
}

async function getContextByFrame(frame: number) {
    return _getContextByFramePath([frame], {root: await getCurrentContext()});
}

async function getContextByFramePath(framePath: number[]) {
    return _getContextByFramePath(framePath, {});
}

async function _getContextByFramePath(framePath: number[], params: Parameters<typeof  browser.browsingContextGetTree>[0]) {
    const {contexts} = await browser.browsingContextGetTree(params);
    const root = contexts[0];
    
    if(!root) throw new Error("Interesting: There should be a root context - shouldn't it?");
    
    let current = root;
    for (let i = 0; i < framePath.length; i++) {
        const index = framePath[i] as number;
        const {children} = current;
        if(children === null) throw new Error("Interesting: when is children null?");
        const child = children[index];
        if(!child) throw new Error(`Frame not found ${JSON.stringify(framePath.slice(0, i+1))}`);
        current = child;
    }
    return current.context;
}

@christian-bromann
Copy link
Member

What would you suggest we should do? Just adding support for selecting a frame by index to make the Chrome Recorder compatible, seems less ideal. On the other side, adding this code to the script in the Chrome Recorder makes also not much sense.

@htho
Copy link
Author

htho commented Jan 10, 2025

Well, the code/function is too complicated and too fragile to add it as a "workaround" to the documentation. (Where people would copy&paste it to their codebase.)
It should be unit/e2e-tested, so we notice when the function breaks.

I assume that I will not be the only one, who wants to use it in the future.

We could add it to the recoder-plugin and export it from there, so it is available for people who need this. But then I would need to pull in all the dependencies, allthough I don't want to do anything with the chrome-recoder.
I don't think we should do that.

We could create an extra package, but that would be insane for ~50 lines of code.

This leaves us with including it in webdriverio.

We could add these as helper functions that return the context id. But I wouldn't know where to add it. It seems uncommon to do that in webdriverio.

I dont think we should add an extra function to browser - there are enough.

This leaves us with an overload for browser.switchFrame.
We could add framePath: number[] and frame: number as deprecated overloads.

Deprecation does not imply something will be removed at some point in the future, it means that we as the developers strongly advice against using it - and I think this is exactly what we want to communicate, don't we?

@christian-bromann
Copy link
Member

An alternative would be to raise an issue with the Recorder extension to provide more meaningful information for the frame element. Wdyt?

@htho
Copy link
Author

htho commented Jan 10, 2025

Of course, we could raise that issue, but I don't think they will consider it.

For a recorder the index approach is good enough. After all a record repeats all steps. In case a step removes an IFrame which would lead to changing indexes, the step will always be executed, the indexes always change in the same order.
It is not a problem for them.

WebdriverIO on the other hand needs an unambiguous way to select a frame. (For tests, where the same steps are repeated over and over again, the index also is sufficient.) BUT: We can not tell if our users run tests or do something else, maybe dynamically. Therefore WE need a better way than indexes.
Also the DX is better when we use selectors for iframe elements. After all, there is no simple way for a developer to find the index of a frame.

Given chromes recorder team probably has enough to do, they will probably not do it - it is just not as important to them, as it is for us.

@christian-bromann
Copy link
Member

@htho let's see what they say ;-)

@christian-bromann
Copy link
Member

@htho it seems you are right and they won't be able to fix this anytime soon. I am more inclined now to enhance the switchWindow command to accept integers that resolve as you described above. If you can provide a PR, I would be happy to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants