GNOME Bugzilla – Bug 788845
Web apps should be more like silos
Last modified: 2018-05-21 16:12:16 UTC
As we discussed on the web engines hackfest, the current web app design is quite broken and leads to several web apps being useless. For many apps we cannot even login properly. We discussed the following changes: - allow tabs: it makes sense for some applications (like google docs), and we can use them as a staging area for links detected as external - be more conservative/forgiving when detecting external links: since we will most likely not be able to detect 100% of the origins that make up the app, be more forgiving giving users a chance to use 'external links' inside the web app and decide whether they want to launch them on the default browser - provide a button in the headerbar for links that look external, so that the user can decide to open in the default browser; that closes the tab I also improved a bit how the "referral" heuristic that exists currently works. A redirect from an uri that seems related to the app also makes the url that got redirected to be considered part of the app - need to see if this is a good idea. It seems to work nicely for twitter, facebook, mattermost.
Created attachment 361348 [details] [review] Do not update the address when a load is ongoing When a redirect happens during a load, the uri tracker triggers an uri update. The load will eventually be committed, leading to the address change. If we update the address when loading, we lose the ability of checking the "current" uri for making policy decisions.
Created attachment 361349 [details] [review] application mode: allow tabs
Created attachment 361350 [details] [review] application mode: track hosts related to the application
Created attachment 361351 [details] [review] application-mode: improve detection of redirects to uris that are part of the app The heuristics that was being used caused problems with logging in to some applications, such as using Phabricator to authenticate to Mattermost. With the availability of tabs for application mode the heuristic is less critical, but it still pays off to provide a good experience to the user by keeping the application-related uris in the same tab.
Created attachment 361352 [details] [review] application mode: add button to open in default browser to non-app uris
Created attachment 361353 [details] [review] application mode: let context menu be more complete, include inspection
Created attachment 361354 [details] [review] application mode: when openning a tab on the default browser, close it
Review of attachment 361348 [details] [review]: Um. Under what situation will the URI change when the view is NOT loading? The view is of course still loading at load committed time.
Review of attachment 361349 [details] [review]: I could have sworn we allowed new tabs in app mode previously. Perhaps this regressed in bug #781440. At the time I approved that patch, I must have forgotten that we were allowing this now, and assumed it was a regression from the switch to GAction. Anyway, from looking at that bug, we see it also needs removed from disabled_actions_for_app_mode in ephy-window.c. So please update that, too. ::: src/ephy-window.c @@ +1848,3 @@ EphyWindow *target_window; + if ((ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()) == EPHY_EMBED_SHELL_MODE_APPLICATION) || Hm, I see that you want to ignore EPHY_PREFS_NEW_WINDOWS_IN_TABS here, but I think EPHY_PREFS_LOCKDOWN_FULLSCREEN should still be respected.
Review of attachment 361350 [details] [review]: ::: embed/ephy-embed-shell.c @@ +166,3 @@ } + g_list_free_full (priv->app_uris, g_free); Better to free this in finalize rather than dispose, since there's no chance of a reference cycle. @@ +1430,3 @@ + **/ +void +ephy_embed_shell_add_app_related_uri (EphyEmbedShell *shell, const char *uri) The second parameter should go on a new line. @@ +1439,3 @@ + g_assert (priv->mode == EPHY_EMBED_SHELL_MODE_APPLICATION); + + /* "Normalize" so we don't save multiple uris with the same origin needlessly. */ This is your indication that priv->app_uris is misnamed. Even though it stores URIs, it should be called priv->app_origins to reflect that everything else has been stripped from the URI. Otherwise it's too hard to reason about what the URIs actually represent; we've had several bugs recently caused by such confusion. Instead of normalizing manually using SoupURI, you should just use ephy_uri_to_security_origin() which does the same thing. Actually, I take that back: it should probably be priv->app_hostnames, constructed using soup_uri_get_host(). Or at least more fields should be unset than just path/query/fragment (how about scheme, port, username, password?). See below. @@ +1445,3 @@ + + base_uri = soup_uri_to_string (soup_uri, FALSE); + if (!g_list_find_custom (priv->app_uris, base_uri, (GCompareFunc) g_strcmp0)) { No braces, since it's a one-line condition. And no space after the cast: (GCompareFunc)g_strcmp0 This check makes me wonder if perhaps GHashTable (with key == value) or GSequence might not be a better data structure, but I guess this list is expected to be very small, so it shouldn't matter much. @@ +1461,3 @@ + **/ +gboolean +ephy_embed_shell_uri_looks_related_to_app (EphyEmbedShell *shell, const char *uri) The second parameter should go on a new line. @@ +1469,3 @@ + + for (GList *iter = priv->app_uris; iter != NULL; iter = iter->next) { + const char* iter_uri = (const char*) iter->data; No space after the cast, and you have the *s in the wrong places too. const char *iter_uri = (const char *)iter->data; @@ +1470,3 @@ + for (GList *iter = priv->app_uris; iter != NULL; iter = iter->next) { + const char* iter_uri = (const char*) iter->data; + if (ephy_embed_utils_urls_have_same_origin (iter_uri, uri)) Hmmm, this function is a bit problematic as it's really checking if the URLs have the same hostname. It should be renamed accordingly, and moved to ephy-uri-helpers.[c,h] instead. (This function has much potential for confusion, so please do fix it, even though I know it was already here before your patch.) This makes me think priv->app_uris should be storing hostnames only (even if still in URI form, the URIs should be stripped of all except the hostname). The protocol/port/username/password etc. are probably not needed, right? ::: src/ephy-shell.c @@ +1233,3 @@ + if (ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()) == EPHY_EMBED_SHELL_MODE_APPLICATION) { + for (int i = 0; uris[i] != NULL; i++) + ephy_embed_shell_add_app_related_uri (ephy_embed_shell_get_default (), uris[i]); So this is only called for the URIs that are passed on the command line, right? i.e. in practice, it's effectively only called for the URI passed to %U in the desktop file?
Review of attachment 361351 [details] [review]: ::: src/ephy-window.c @@ +1932,1 @@ + /* If the application's origin does a server redirect to an external origin, it is likely I think this comment should go inside the third condition block, right? @@ +1945,2 @@ + /* This does not look like a part of the application itself, so open it in a new tab + * which will provide a way of openning in the default browser. opening How does this work, and why is it better than calling ephy_file_open_uri_in_default_browser() directly?
(In reply to Michael Catanzaro from comment #11) > Review of attachment 361351 [details] [review] [review]: > @@ +1945,2 @@ > + /* This does not look like a part of the application itself, so open > it in a new tab > + * which will provide a way of openning in the default browser. … > How does this work, and why is it better than calling > ephy_file_open_uri_in_default_browser() directly? It's what we discussed we should do in the break-out session. This works like the twitter and facebook phone apps and gets us 2 nice features: - content is closer to what originated it, which is good if it's a short read - most importantly, though, it is conservative enough to avoid issues even when the heuristics make mistakes trying to figure out whether the link is part of the app or not - you can still log in to the application if the login page is seen as external We even discussed whether the heuristics would even be needed at all in this case. I think we should keep some and improve as we learn of new ways to do so, so that the experience is as slick as possible. But being conservative while allowing the user to 'take this outside' is the way to go, I think.
(In reply to Michael Catanzaro from comment #10) > Review of attachment 361350 [details] [review] [review]: > + /* "Normalize" so we don't save multiple uris with the same origin > needlessly. */ > > This is your indication that priv->app_uris is misnamed. Even though it > stores URIs, it should be called priv->app_origins to reflect that > everything else has been stripped from the URI. Otherwise it's too hard to > reason about what the URIs actually represent; we've had several bugs > recently caused by such confusion. Instead of normalizing manually using > SoupURI, you should just use ephy_uri_to_security_origin() which does the > same thing. Ah, that's good, I failed to find that one. And indeed, I had started with uris, but realized it would not make sense to add uris since all we need is the origin. > Actually, I take that back: it should probably be priv->app_hostnames, > constructed using soup_uri_get_host(). Or at least more fields should be > unset than just path/query/fragment (how about scheme, port, username, > password?). See below. I also thought about using get_host(), but… > @@ +1470,3 @@ > + for (GList *iter = priv->app_uris; iter != NULL; iter = iter->next) { > + const char* iter_uri = (const char*) iter->data; > + if (ephy_embed_utils_urls_have_same_origin (iter_uri, uri)) > > Hmmm, this function is a bit problematic as it's really checking if the URLs > have the same hostname. It should be renamed accordingly, and moved to > ephy-uri-helpers.[c,h] instead. (This function has much potential for > confusion, so please do fix it, even though I know it was already here > before your patch.) … this function, despite the name of the soup API it ends up uses, does test for origins - including at least the scheme and port. And I think it might make sense to keep those in the check - although user and passwords probably not. Port may be particularly important as people may run different apps on different ports. Crazy corner case, but… > ::: src/ephy-shell.c > @@ +1233,3 @@ > + if (ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()) == > EPHY_EMBED_SHELL_MODE_APPLICATION) { > + for (int i = 0; uris[i] != NULL; i++) > + ephy_embed_shell_add_app_related_uri (ephy_embed_shell_get_default > (), uris[i]); > > So this is only called for the URIs that are passed on the command line, > right? i.e. in practice, it's effectively only called for the URI passed to > %U in the desktop file? Basically, yes. I also pondered about adding a cmd line switch in which you could manually specify uris just for this list rather than for opening. But maybe we should leave that for later.
(In reply to Michael Catanzaro from comment #8) > Review of attachment 361348 [details] [review] [review]: > > Um. Under what situation will the URI change when the view is NOT loading? > The view is of course still loading at load committed time. It changes when the history API is used, or when in-page anchors are clicked. EphyWebView's address is explicitly set to the new URI when the load is committed: case WEBKIT_LOAD_COMMITTED: { const char *uri; view->ever_committed = TRUE; /* Title and location. */ uri = webkit_web_view_get_uri (web_view); ephy_web_view_set_committed_location (view, uri); <-- this calls set_address update_security_status_for_committed_load (view, uri);
(In reply to Gustavo Noronha (kov) from comment #14) > (In reply to Michael Catanzaro from comment #8) > > Review of attachment 361348 [details] [review] [review] [review]: > > > > Um. Under what situation will the URI change when the view is NOT loading? > > The view is of course still loading at load committed time. > > It changes when the history API is used, or when in-page anchors are > clicked. EphyWebView's address is explicitly set to the new URI when the > load is committed: > > case WEBKIT_LOAD_COMMITTED: { > const char *uri; > view->ever_committed = TRUE; > > /* Title and location. */ > uri = webkit_web_view_get_uri (web_view); > ephy_web_view_set_committed_location (view, uri); <-- this calls > set_address > update_security_status_for_committed_load (view, uri); I'm still concerned about this patch, since it means the uri property and the address property will be desynced during the redirect. I think this could lead to strange bugs in the future. It's already confusing enough that address (the EphyWebView property) is different than uri (the WebKitWebView property) without the address becoming different at crucial points. I don't want to outright reject this, but consider finding another workaround to try to make the original URI available in the policy decision callback. In particular, it's probably bad that replacing your call to ephy_web_view_get_address() in the policy decision callback with webkit_web_view_get_uri() would lead to incorrect results. I'm tired and having a hard time thinking of good alternatives right now, but I bet we can find one. We could add a redirected-uri property to EphyWebView or maybe even to WebKitURIRequest, for example. Alternatively: it's a bit frustrating that we don't have the WebKitURIResponse of the redirect available in the policy decision callback, like it is in WebKitWebPage::send-request. I wonder how hard it would be to add that to WebKitNavigationPolicyDecision. But maybe those are all bad ideas. (In reply to Gustavo Noronha (kov) from comment #13) > … this function, despite the name of the soup API it ends up uses, does test > for origins - including at least the scheme and port. Wow, indeed. I guess we can stick with security origins then, if that's your preference. (In reply to Gustavo Noronha (kov) from comment #12) > (In reply to Michael Catanzaro from comment #11) > > Review of attachment 361351 [details] [review] [review] [review]: > > @@ +1945,2 @@ > > + /* This does not look like a part of the application itself, so open > > it in a new tab > > + * which will provide a way of openning in the default browser. > … > > How does this work, and why is it better than calling > > ephy_file_open_uri_in_default_browser() directly? > > It's what we discussed we should do in the break-out session. This works > like the twitter and facebook phone apps and gets us 2 nice features: Ah yeah, that's right. I had confused myself. ;)
Review of attachment 361352 [details] [review]: Code looks fine. I really ought to test it before landing, but that won't happen today. In the meantime... how about a screenshot?
Review of attachment 361353 [details] [review]: Yeah, this is just annoying.
Review of attachment 361354 [details] [review]: openning -> opening again (in the commit message, this time) Otherwise, looks good. Once you post a new set of patches, it'll be a few days before I can give a final review, but it looks like this is all really close.
(In reply to Michael Catanzaro from comment #9) > Review of attachment 361349 [details] [review] [review]: > Anyway, from looking at that bug, we see it also needs removed from > disabled_actions_for_app_mode in ephy-window.c. So please update that, too. Funnily enough it seems to not be there already? > ::: src/ephy-window.c > @@ +1848,3 @@ > EphyWindow *target_window; > > + if ((ephy_embed_shell_get_mode (ephy_embed_shell_get_default ()) == > EPHY_EMBED_SHELL_MODE_APPLICATION) || > > Hm, I see that you want to ignore EPHY_PREFS_NEW_WINDOWS_IN_TABS here, but I > think EPHY_PREFS_LOCKDOWN_FULLSCREEN should still be respected. Application mode is enforcing "no new windows, always use tabs" itself, which is what EPHY_PREFS_LOCKDOWN_FULLSCREEN being true achieves in this case as well (it being false only means 'use your default new windows policy', which for us would be the same as it being true). Or do I misunderstand what you mean here?
Created attachment 361448 [details] The button to open in browser There goes the screenshot =)
Comment on attachment 361353 [details] [review] application mode: let context menu be more complete, include inspection Attachment 361353 [details] pushed as d8af8e4 - application mode: let context menu be more complete, include inspection
Created attachment 361456 [details] [review] Track the currently committed address While doing a load, some policy decisions may depend on where we are coming from. Both WebKitWebView's uri and EphyWebView's address change even for ongoing loads, so we track the last committed uri.
Created attachment 361457 [details] [review] application mode: allow tabs
Created attachment 361458 [details] [review] application mode: track origins related to the application
Created attachment 361459 [details] [review] application-mode: improve detection of redirects to uris that are part of the app The heuristics that was being used caused problems with logging in to some applications, such as using Phabricator to authenticate to Mattermost. With the availability of tabs for application mode the heuristic is less critical, but it still pays off to provide a good experience to the user by keeping the application-related uris in the same tab.
Created attachment 361460 [details] [review] application mode: add button to open in default browser to non-app uris
Created attachment 361461 [details] [review] application mode: when opening a tab on the default browser, close it
Review of attachment 361456 [details] [review]: ::: embed/ephy-web-view.c @@ +2899,3 @@ + * @view: an #EphyWebView + * + * Returns the address of the last fully loaded page, percent-encoded. This doc comment is obviously wrong, since the last committed address is of course set in LOAD_COMMITTED, not LOAD_FINISHED.
Review of attachment 361460 [details] [review]: ::: src/ephy-header-bar.c @@ +775,3 @@ + /* Open in browser */ + if (ephy_embed_shell_get_mode (ephy_embed_shell_get_default ())) { This test is broken since you forgot to compare against EPHY_EMBED_SHELL_MODE_APPLICATION, which is causing normal browser mode to crash immediately. So be sure to test browser mode too, please. ;) @@ +790,3 @@ + + context = gtk_widget_get_style_context (button); + gtk_style_context_add_class (context, "suggested-action"); Let's try without suggested-action for now, since I'm not sure I like how it looks here.
Review of attachment 361460 [details] [review]: ::: src/ephy-header-bar.c @@ +775,3 @@ + /* Open in browser */ + if (ephy_embed_shell_get_mode (ephy_embed_shell_get_default ())) { This is really weird as I am installing it and using it for the default browser as well. I wonder why it wasn't crashing here… anyway, will do!
Review of attachment 361458 [details] [review]: ::: embed/ephy-embed-shell.c @@ +1509,3 @@ + + g_assert (EPHY_IS_EMBED_SHELL (shell)); + g_assert (priv->mode == EPHY_EMBED_SHELL_MODE_APPLICATION); FWIW this is the assert it's crashing on for me. Anyway, not a big deal as we know how to fix it.
Attachment 361456 [details] pushed as 34216ae - Track the currently committed address Attachment 361457 [details] pushed as 1b8facf - application mode: allow tabs Attachment 361458 [details] pushed as 57c863c - application mode: track origins related to the application Attachment 361459 [details] pushed as f0e4daa - application-mode: improve detection of redirects to uris that are part of the app Attachment 361460 [details] pushed as 0c3e4b2 - application mode: add button to open in default browser to non-app uris Attachment 361461 [details] pushed as 26bb484 - application mode: when opening a tab on the default browser, close it
Sorry for the delay, I had some busy days last week =)
No problem, thanks for working on this! Reopening because this accidentally added a dependency on WebKit trunk... I'll push an #if WEBKIT_CHECK_VERSION guard, then after the 2.19.1 release, let's remove the guard, bump our WebKit dep, and close this bug.
(In reply to Michael Catanzaro from comment #34) > No problem, thanks for working on this! > > Reopening because this accidentally added a dependency on WebKit trunk... > I'll push an #if WEBKIT_CHECK_VERSION guard, then after the 2.19.1 release, > let's remove the guard, bump our WebKit dep, and close this bug. Forgot to close it.
Gustavo, please look at https://gitlab.gnome.org/GNOME/gnome-software/issues/390