squalou / google-chat-linux

source of a fork of google-chat-linux unofficial client from robyf
50 stars 10 forks source link

Clicking on link doesn't open them in browser #34

Closed vwbusguy closed 3 years ago

vwbusguy commented 3 years ago

Clicking on links in the chat doesn't appear to do anything (nothing in Console or Net with DevTools pane and nothing in stdout). If I right-click on the link, I get a Copy Link option, so I can copy and paste it to a browser, but clicking on the link itself does not open the link to the browser.

The option for this is enabled: image

If it's helpful, I'm running Fedora 34 with Firefox as the default browser.

squalou commented 3 years ago

That's weird. I'm using Firefox too as default browser and it works, using arch + xfce.

In this situation, Electron uses shell.openExternal(url), maybe something prevents it from finding what action / program should be called.

There's not much expained here https://www.electronjs.org/docs/api/shell#shellopenexternalurl-options

Just in case, can you try this xdg-open www.duckduckgo.com in a terminal and see if the default browser opens ? (I bet it does but you never know)

vwbusguy commented 3 years ago

xdg-open 'https://www.duckduckgo.com' works for me. Opens a tab in Firefox. Note that I had to add the protocol from your example. When I copy URL, it includes the protocol, so I doubt that's the issue.

So I decided to tail out journald and it spits out these two log entries when I click on a link:

Apr 22 12:08:41 samus systemd[2164]: Started Application launched by gio open.
Apr 22 12:08:41 samus systemd[2164]: Started cgroupify@app-glib-firefox-120559.scope.service.

No errors though, so not much to go on, except that it seems to be trying to call Firefox and quietly failing. FWIW, I have slack installed on here, which I believe is also an electron app under the hood, and it can open links fine.

vwbusguy commented 3 years ago

So, this is interesting, I restarted it with strace to see if I could get any more clues and then the links worked. Restarting it without strace and the links quietly fail to launch again.

squalou commented 3 years ago

Wow, that's indeed interesting, but no idea how to use this info to fix the issue.

There is a suggestion in this stackoverflow topic :

https://stackoverflow.com/questions/32402327/how-can-i-force-external-links-from-browser-window-to-open-in-a-default-browser

would be to try to use

var openBrowser(url) {
  require('child_process').exec('xdg-open ' + url);
}

instead of the regular shell.openExternal(url); Would be worth a try if you can.

(at least to see if the behaviour is the same)

Things to change are in window.js, line 228

vwbusguy commented 3 years ago

That works! Not sure why shell.openExternal(url) seems to have some weird race condition, but the child_process exec method worked against all the links I tried.

        } else if ( ! openUrlInside && ! doNotRedirect(url)){
                //shell.openExternal(url);
                require('child_process').exec('xdg-open ' + url);
                e.preventDefault();
        }

I'm a little concerned about the exec formatting here allowing for potential shell injection.

I tried a few and it looks like the front end validates against semi-colons, encoded or not, so might not be an issue.

vwbusguy commented 3 years ago

If you want, I'm happy to submit a PR for it.

squalou commented 3 years ago

Good news ! Don't worry with the PR : I'll first have to make some checks on other OS'es (for some friends using windows at work) So .. either I'll add an OS-dependant ugly 'if' or yet another option in a hidden menu (even uglier)

I hope you can use you own patched version for a few days, time for me to test that.

squalou commented 3 years ago

well, I have some situations where using xdg-open on some urls will trigger this warning :

image

I face that on internal-use urls that indeed do redirections.

This warning does'nt show up when using openExternal.

So... I'll go for the additional Tickbox in 'avdanced" submenu.

squalou commented 3 years ago

@vwbusguy can you pull master and give it a try ? I'll try on windows to be sure it doesn't break anything before releasing.

vwbusguy commented 3 years ago

I did a git stash, git pull, and an npm install for good measure, and the links now open fine for me! Many thanks!

squalou commented 3 years ago

Hi @vwbusguy I just realized this morning that the 'tickbox' state didn't survive the app restart. Sorry about that, fixed in 5.12.11-3