Skip to content

fix: give a same-service window.open a real window, not nil - #18

Open
marcioviniciusspiridigliozzi-dot wants to merge 1 commit into
nicojan:mainfrom
marcioviniciusspiridigliozzi-dot:fix/window-open-handle
Open

fix: give a same-service window.open a real window, not nil#18
marcioviniciusspiridigliozzi-dot wants to merge 1 commit into
nicojan:mainfrom
marcioviniciusspiridigliozzi-dot:fix/window-open-handle

Conversation

@marcioviniciusspiridigliozzi-dot

Copy link
Copy Markdown
Contributor

A new-window request whose target belongs to the opening service was collapsed into the opener's web view, and the delegate returned nil.

For a link click that is right, and it has to stay that way — Slack's target=_blank workspace links go through this path and should keep loading in place rather than spawning a window each time.

For a programmatic window.open() it is wrong. The caller gets a window handle back, and callers test it:

const w = window.open(url);
if (!w) return;

Returning nil reads as "popup blocked", so the page abandons whatever it was starting with no window and no error to show for it. Nothing in the UI says a request was refused, which makes it a hard thing to diagnose from the outside.

The change

Narrow the collapse to .linkActivated rather than removing it. A same-service window.open now falls through to a real window, which shares the opener's data store, so a session started in it lands in the right place.

The rule moves into shouldLoadNewWindowInPlace so it can be tested without a live WKWebView — covered for the clicked same-service link (still collapses), the programmatic same-service popup (gets a window), the cross-service case, and unknown hosts on either side.

Scope, stated plainly

I first reached this while chasing a Teams sign-in failure and believed it was the cause. It was not. Logging createWebViewWith showed it was never called during that failure — zero times — and the real cause turned out to be ITP blocking a third-party cookie, which I have sent separately in #17.

This change stands on its own as a correctness fix for window.open. It is not a fix for that bug, and I would rather say so than let the connection be assumed from the timing of the two PRs.

Note on CI

This branches from main, which currently has an intermittent StoreRepair test flake; I sent a fix for it in #15. A red run here is most likely that rather than this change.

A new-window request whose target belongs to the opening service was
collapsed into the opener's web view, and the delegate returned nil.

For a link click that is right, and it has to stay that way: Slack's
target=_blank workspace links go through this path and should keep
loading in place rather than spawning a window each time.

For a programmatic window.open() it is wrong. The caller gets a window
handle back, and callers test it:

    const w = window.open(url); if (!w) return;

Returning nil reads as "popup blocked", so the page abandons whatever it
was starting with no window and no error to show for it. Nothing in the
UI says a request was refused, which makes it a hard thing to diagnose
from the outside.

So narrow the collapse to .linkActivated rather than removing it. A
same-service window.open now falls through to a real window, which
shares the opener's data store, so a session started in it lands in the
right place.

The rule moves into shouldLoadNewWindowInPlace so it can be tested
without a live WKWebView.

Scope, stated plainly: I first reached this while chasing a Teams
sign-in failure and believed it was the cause. It was not — logging
createWebViewWith showed it was never called during that failure, and
the real cause was ITP blocking a third-party cookie, which I have sent
separately. This change stands on its own as a correctness fix for
window.open; it is not a fix for that bug, and I would rather say so
than let the connection be assumed.
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.

1 participant