Viewing /help page from within app is confusing

[jrowan]

When I visited the /help page for keyboard shortcuts within the app, I thought I was trapped for a while and ended up getting out in a very non-intuitive way by clicking log in in the upper right hand corner of that chat page (even though I was already logged in).

Steps to reproduce:

1. hit ? to open the keyboard shortcuts help modal
2. click the "detailed keyboard shortcuts documentation" link
3. This page should appear
4. hitting the 'home' arrow takes you to the user documentation home page, hitting 'login' should take you back to chat

[jrowan]

Confirmed that I'm still getting the "log in" button (as the only way to get out) when I click a user documentation link from within the app. On the web app the page also shows a "log in" button even when I'm logged in in another tab, but it's not as bad for navigation since it's in a separate tab.

[timabbott]

Good find. @akashnimare this is basically the same issue is the /apps page. I think we might want to invert the approach for the desktop app, by having a whitelist of what pages open inside the app. A good rule might be this:

• If the URL starts with /accounts, /register, or /login or is the root URL (potentially with a narrow), open it in-app. Otherwise, open in a browser.

Can you try that out? I believe that everything needed for the login process is in those URL domains, and unless I'm missing something, that's really the only thing other than the Zulip webapp experience itself that should be opened in-app.

[akashnimare]

@timabbott so we need to open the /help page in the default browser as well?

[timabbott]

Correct. But we'll we'll probably over time add many more pages to the Zulip documentation sites, so I think we actually want to have a whitelist for what doesn't get opened in the default browser.

[jrowan]

What about /user_uploads? For mp4 videos I also get stuck and unable to get out, but pictures are working fine/as expected.

[timabbott]

When you say "pictures are working fine/as expected", I assume you mean the lightbox UI works? That's great, but the lightbox UI doesn't navigate the main URL of the page; it just fetches the external files using JS/HTML within the Zulip web app. What we're discussing here is the main URL of the page changing through clicking on a link.

[geeeeeeeeek]

@timabbott Maybe we should make a whitelist of open-in-browser urls, rather than a list of in-app urls. It's more consistent with the current behavior. We don't want to unexpectedly redirect (maybe a new url pattern) to browser as user clicks something.

So, as far as I know, the whitelist should include /help, /apps, /integrations, /api and stats, right?

[geeeeeeeeek]

Actually we have a filter now:

We can add /help, /apps and stats here as well.

[geeeeeeeeek]

@timabbott @akashnimare

Update:
On second thought, why do we need the isInternal on new-window at the first place? The event will only be triggered when a link tries to open a new tab. We can just follow the original behavior! Is there any reason why we added the isInternal? Is it because if we open Statistics in browser, the session will be broken?

[timabbott]

My concern is that we'll keep adding new landing page type content to the Zulip webapp that one might send a link to, and clicking that link in the app shouldn't open it. That list of URLs will change frequently with time.

We do need to whitelist the login/user registration flow pages, or you won't be able to login :). But those are essentially all within a nice, consistent, URL namespace starting with /account.

The reason I was thinking a whitelist is that I think essentially nothing should open in-app other than direct navigation within the app and the process for logging into the app.

[akashnimare]

On second thought, why do we need the isInternal on new-window at the first place?

@geeeeeeeeek we need new-window event because it's necessary to handle links which contains target="_blank". If we don't handle this event those links will be opened in a new browserwindow.

[geeeeeeeeek]

@akashnimare Yup, what if we keep new-window event but discard isInternal? Let all links with target="_blank" open in browser?

The only problem I found, is that when /stats opens in browser, the session will be lost and will redirect to login page. However, I don't think it should be opened in the app...

[timabbott]

Yeah, the session problem is sorta a necessary evil. We may later move that page to be in-app to resolve that concern, but with the whitelist approach I suggested, that wouldn't require any changes to the desktop app.

[akashnimare]

Fixed in zulip/zulip-electron@9e4e5e9. From now on, pages like /help, /stats, /api, /integrations etc would get open in the default browser.