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.

Sign up for GitHub

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

fix: webwallet trpc link for popups #164

Merged
merged 2 commits into from
Dec 19, 2024
Merged

fix: webwallet trpc link for popups #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25c10c6
fix: webwallet trpc link for popups
bluecco Dec 19, 2024
2f261ff
chore: update release.yml
bluecco Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ on:
branches:
- develop
- main
- beta-braavos-mobile
- beta-trpc-link
- hotfix\/v[0-9]+.[0-9]+.[0-9]+

jobs:
Expand Down
78 changes: 57 additions & 21 deletions src/connectors/webwallet/helpers/trpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,36 @@ export const setPopupOptions = ({
popupParams = `width=${width},height=${height},top=${y},left=${x},toolbar=no,menubar=no,scrollbars=no,location=no,status=no,popup=1`
}

const PopupManager = {
currentPopup: null as Window | null,

createPopup(url: string, name: string, params: string): Window {
// Close any existing popup
if (this.currentPopup && !this.currentPopup.closed) {
this.currentPopup.close()
}

// Create popup immediately
const popup = window.open(url, name, params)

if (!popup) {
throw new Error("Popup blocked by browser")
}

this.currentPopup = popup

// Monitor popup state
const checkClosed = setInterval(() => {
if (popup.closed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if popup.closed === true but this.currentPopup !== popup, aka we got another popup 🤔
I'm just thinking of the possible corner case here, not stating that is an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be an edge case, but theoretically this.currentPopup !== popup should not happen in this case
popup is always the new one

thinking if this approach could be more reliable but it would requires some tests (using beta branch)

clearInterval(checkClosed)
this.currentPopup = null
}
}, 500)

return popup
},
}

// TODO: abstract AppRouter in order to have one single source of truth
// At the moment, this is needed
const appRouter = t.router({
Expand Down Expand Up @@ -195,29 +225,35 @@ export const trpcProxyClient = ({
false: popupLink({
listenWindow: window,
createPopup: () => {
let popup: Window | null = null
const webwalletBtn = document.createElement("button")
webwalletBtn.style.display = "none"
webwalletBtn.addEventListener("click", () => {
popup = window.open(
`${popupOrigin}${popupLocation}`,
"popup",
popupParams,
)
})
webwalletBtn.click()

// make sure popup is defined
;(async () => {
while (!popup) {
await new Promise((resolve) => setTimeout(resolve, 100))
}
})()
try {
let popup: Window | null = null
const button = document.createElement("button")
button.style.position = "fixed"
button.style.top = "-999px"
button.style.left = "-999px"
button.style.opacity = "0"
button.style.pointerEvents = "none" // prevent click event
Comment on lines +231 to +235
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these styles making any difference from the previously used webwalletBtn.style.display = "none"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did some searching and it seems it's more reliable

  • Browser Security: Some browsers treat elements with display: none as "not truly interactive" from a security perspective, which can affect how they handle popup permissions
  • DOM Interaction: While display: none removes the element from the layout flow completely, position: fixed keeps it in the DOM's interactive layer
  • Mobile Browsers: Particularly on mobile browsers, clicks on display: none elements might be ignored or treated differently than clicks on positioned-but-invisible elements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I saw something like this coming, legit 👍🏼


if (!popup) {
throw new Error("Could not open popup")
button.addEventListener("click", () => {
popup = PopupManager.createPopup(
`${popupOrigin}${popupLocation}`,
"popup",
popupParams,
)
})

document.body.appendChild(button)
button.click()
button.remove()

if (!popup) {
throw new Error("Could not open popup")
}
return popup
} catch (error) {
console.error("Failed to create popup:", error)
throw error
}
return popup
},
postOrigin: "*",
}),
Expand Down
Loading