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

discoverProxyCandidates example code does not find all candidates correctly #1468

Open
1 of 7 tasks
Roaders opened this issue Dec 12, 2024 · 3 comments · May be fixed by #1466
Open
1 of 7 tasks

discoverProxyCandidates example code does not find all candidates correctly #1468

Roaders opened this issue Dec 12, 2024 · 3 comments · May be fixed by #1466
Labels
bug Something isn't working

Comments

@Roaders
Copy link
Contributor

Roaders commented Dec 12, 2024

Area of Issue

  • App Directory
  • API
  • Context Data
  • Intents
  • Desktop Agent Bridging
  • Use Cases
  • Other

Issue Description:

The sample code for discovering proxy candidates here:

https://fdc3.finos.org/docs/next/api/specs/webConnectionProtocol#12-desktop-agent-discovery

Is not complete. It does not add parents of the opener. These parent could also have openers of course. We actually need a recursive function I think. Something like:

export function discoverProxyCandidates(windowRef?: WindowProxy): WindowProxy[] {
    const candidates: WindowProxy[] = [];

    return addCandidates(windowRef ?? window, candidates);
}

function addCandidates(window: WindowProxy, candidates: WindowProxy[]): WindowProxy[] {
    candidates.push(window);

    if (window.opener != null) {
        addCandidates(window.opener, candidates);
    }

    if (window.parent != window) {
        addCandidates(window.parent, candidates);
    }

    return candidates;
}
@Roaders Roaders added the bug Something isn't working label Dec 12, 2024
@kriswest
Copy link
Contributor

I believe that this was deliberate at the time - we had considered a need for searching up the tree of parents and to any opener of each, but not parents of the opener. However,whats in the current implementation PR(s) does search for parents of the opener via recursion:

/**
 * Recursive search for all possible parent frames (windows) that we may
 * target with the WCP.
 * @param startWindow window object to search
 * @param found window objects found so far
 */
function collectPossibleTargets(startWindow: Window, found: Window[]) {
  _recursePossibleTargets(startWindow, startWindow, found);
  Logger.debug(`Possible parent windows/frames found: ${found.length}`);
}

function _recursePossibleTargets(startWindow: Window, w: Window, found: Window[]) {
  if (w) {
    if (found.indexOf(w) == -1 && w != startWindow) {
      found.push(w);
    }

    if (!!w.opener) {
      _recursePossibleTargets(startWindow, w.opener, found);
    }

    if (w.parent != w) {
      _recursePossibleTargets(startWindow, w.parent, found);
    }
  }
}

which is similar to what you propose - with one key difference, it won't return the window you start with and hence won't send messages to the app window.

If you see a use case for the DA to be the parent of an opening iFrame I'm happy to raise a PR to replace that function in the docs.

@kriswest kriswest linked a pull request Dec 16, 2024 that will close this issue
16 tasks
@Roaders
Copy link
Contributor Author

Roaders commented Dec 18, 2024

We discussed this today and I think we do have a use case that requires deep window hierarchies (and would need a recursive search for parents using window.opener).

We have apps that have quite complex structures with many deeply nested iframes and tabs and so on. It is very likely that some of these web pages will open a new window with window.open() rather than agent.open(). In this scenario we want the grandchild window to still be able to establish communication with the root window.

@kriswest
Copy link
Contributor

kriswest commented Dec 18, 2024

@Roaders in anticipation I've updated the code snippet to what we've used in the implementation in PR #1466

```ts
/**
* Recursive search for all possible parent frames (windows) that we may
* target with the WCP.
* @param startWindow window object to search
* @param found window objects found so far
* @return An array of Window Objects representing 'parent' frames of the
* specified window.
*/
function discoverProxyCandidates(startWindow: Window, found: Window[]) {
_recursePossibleTargets(startWindow, startWindow, found);
Logger.debug(`Possible parent windows/frames found: ${found.length}`);
return found;
}
function _recursePossibleTargets(startWindow: Window, w: Window, found: Window[]) {
if (w) {
if (found.indexOf(w) == -1 && w != startWindow) {
found.push(w);
}
if (!!w.opener) {
_recursePossibleTargets(startWindow, w.opener, found);
}
if (w.parent != w) {
_recursePossibleTargets(startWindow, w.parent, found);
}
}
}
```

We hope to merge that PR before tomorrow's meeting into fdc3-for-web-impl, then fdc3-for-web-impl into main as soon as we can get reviews thereafter (fingers crossed).

@kriswest kriswest reopened this Dec 19, 2024
@kriswest kriswest linked a pull request Dec 28, 2024 that will close this issue
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants