Skip to content

fix: stop an ordinary link popup reloading the service behind it - #21

Open
marcioviniciusspiridigliozzi-dot wants to merge 1 commit into
nicojan:mainfrom
marcioviniciusspiridigliozzi-dot:fix/popup-close-reloads-service
Open

fix: stop an ordinary link popup reloading the service behind it#21
marcioviniciusspiridigliozzi-dot wants to merge 1 commit into
nicojan:mainfrom
marcioviniciusspiridigliozzi-dot:fix/popup-close-reloads-service

Conversation

@marcioviniciusspiridigliozzi-dot

Copy link
Copy Markdown
Contributor

Closing a popup reloads the service that opened it, whatever the popup was.

That is right for sign-in. The popup shares the service's data store, so once sign-in finishes the session cookies are already there and the main view only needs a reload to leave its signed-out page.

But a popup is also how an ordinary link opens. Glance at a link from a chat service, close the window, and the service reloads underneath you — scroll position gone, a half-typed message gone, whatever the page held but never sent gone. A steady, visible cost, paid for a case that comes up rarely.

The change

Two signals separate the two, and either is enough:

  • The page closed itself. OAuth popups finish by calling window.close(); a link window is closed by the user. These arrive at different delegate callbacks, so this is not a guess. It is also what carries flows through providers not listed by name, such as a company's own Okta or Keycloak.

  • The popup was opened at a known sign-in gateway. A service asking the user to sign in again opens straight at its provider, so the first URL is the gateway. This covers a flow the user closes by hand once it is done, which some providers leave to them.

When neither holds, the popup was a link, and the service is left alone.

Why the second signal reads only the opening URL

I had it watching the whole navigation chain first, and measured that being wrong before sending this.

Opening an Azure portal link from Teams starts at portal.azure.com and redirects through login.microsoftonline.com for SSO. Watching the chain counted that as a sign-in and reloaded Teams on close — the exact bug this PR is about, surviving the fix meant to remove it. I added a temporary log line and reproduced it:

POPUPCLOSE selfClosed=false openedAtAuthHost=true reload=true

Reading only the opening URL keeps the re-auth case, which opens at the gateway, and drops the link case, which merely passes through it. Both are covered by tests; the second is named for how it was found, and the reasoning sits in the doc comment so the chain-watching version does not get restored later as an improvement.

Verified after the change on the same machine: opening that link and closing it now leaves Teams alone.

The trade

Stated plainly, because it is a real one. A sign-in that neither starts at a listed gateway nor closes itself leaves the service on its signed-out page until the user hits reload, once. Against reloading on every link close, that seemed the better way round — but if you would rather have the old behaviour behind a per-service option, say so and I will do that instead.

The rule is factored into shouldReloadOpener so it can be tested without a live WKWebView. Five tests; the temporary probe is gone.

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.

Closing a popup reloaded the service that opened it, whatever the popup
was. That is right for sign-in and wrong for everything else.

Reloading exists because the popup shares the service's data store: once
sign-in finishes the session cookies are already here, and the main view
only needs a reload to leave its signed-out page.

But a popup is also how an ordinary link opens. Glance at a link from a
chat service, close the window, and the service reloads underneath you —
scroll position gone, a half-typed message gone, whatever the page held
but never sent gone. A steady, visible cost, paid for a case that comes
up rarely.

Two signals separate them, and either is enough:

  - The page closed itself. OAuth popups finish by calling
    window.close(); a link window is closed by the user. This is what
    carries flows through providers not listed by name, such as a
    company's own Okta or Keycloak.

  - The popup was opened at a known sign-in gateway. A service asking
    the user to sign in again opens straight at its provider, so the
    first URL is the gateway. This covers a flow the user closes by hand
    once it is done, which some providers leave to them.

The second signal reads the opening URL only, never the rest of the
navigation chain. I had it watching the whole chain first, and measured
that being wrong: opening an Azure portal link from Teams starts at
portal.azure.com and redirects through login.microsoftonline.com for
SSO, so the chain marked it a sign-in and reloaded Teams on close — the
very bug this commit is about. Reading only the opening URL keeps the
re-auth case (which opens at the gateway) and drops the link case. Both
are covered by tests, the second named for how it was found.

When neither signal holds, the popup was a link, and the service is left
alone. The signal is cleared with the rest of the popup state, so a link
opened after a sign-in does not inherit its predecessor's reason to
reload.

The trade this makes is mild and recoverable in the direction it fails:
a sign-in that neither starts at a listed gateway nor closes itself
leaves the service on its signed-out page until the user hits reload,
once. Against reloading on every link close, that is the better way
round.

The rule is factored into shouldReloadOpener so it can be tested without
a live WKWebView.
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