GNOME Bugzilla – Bug 658395
Escaped from "Application mode"
Last modified: 2014-10-14 13:23:45 UTC
1. Go to http://www.netvibes.com 2. Read one of the items 3. Press "L" to open the actual link It will then open a second window, from which you can browse, instead of launching epiphany itself.
epiphany-3.1.90-1.fc16.x86_64
Created attachment 196342 [details] example Apparently, window.open() or any other javascript method that opens a link can trigger this. The attached example shows that.
Created attachment 197676 [details] [review] app-mode: Load pages in external hosts in a new ephy instance This is a bit hacky solution, implemented by halting the load during the provisional loading state. We need this because javascript-triggered page loads do not provide enough context during the policy decision request. This doesn't solve the problem for window.open() calls
The fix above is far from what I'd like, but I fail to see how, with the context we get from webkit, we could do it in a similar way as it's currently implemented for regular links. For one, js opened links trigger the policy decision request with WEBKIT_WEB_NAVIGATION_REASON_OTHER, which also can mean tons of other things, so it's not enough. Second, WebKit is not even requesting a policy decision regarding new windows when these are requested via window.open() calls. There is a WebKit bug about it already: https://bugs.webkit.org/show_bug.cgi?id=23432 IMHO, ideally we would fix the bug above and add a new WebKitWebNavigationReason for JavaScript-triggered navigation actions. But I don't know how feasible is that.
Also, the patch above solves the netvibes case, but is not very nice for facebook links which have the following sequence: window.open("fb.sm/13w4") -> location.url = actualtarget.com.
*** Bug 666362 has been marked as a duplicate of this bug. ***
We have some scenarios that fail, and others that don't fail currently, but could if we fix one of the existing issues: a) App uses an external authentication service, and you access to it using a "Sign in" link. App mode will open default web browser for doing login, and so, you can never do login inside the sandbox of the application. b) App uses an external authentication service, but you get redirected, no explicit link tap. App mode will open the external web, and it'll work as login happens in the same sandbox and pages flow. c) App hides external links with "hit counters" or "secure links". These are links to the same origin, that do some check or store some data, and then open the external link. Currently user would expect these to be opened on default browser, but will be opened in the app mode view. If we fix this adding checks for javascript redirects or the actual page being opened (load_status_cb), then we'll break case (b). My proposal: Behaviour of "open link", and "open link through redirect or javascript" works as in browser. But there are some changes: * We show the address bar, but it is not editable. * We add a button in the address bar to open the web in the default web browser. * Maybe show some warning that user could dismiss telling the address is outside the application. * The most obvious win of this approach is that we keep the back-forward flow in this case. So user opens a link, reviews its contents, and closes it to go on working with the app. New contextual action for links "open in web browser", replacing "open in tab/window" action. This should also be the action for middle click. So, in case user wants to use the other flow directly, without tapping on the button to "show in browser", user saves a step. By the way, checking the current design proposal for the URL toolbar in https://live.gnome.org/Design/Apps/Web I think it will also help for application mode: * In application mode we would never show the editable URL. We would need solution for providing "refresh" and "stop" anyway... * No "pages" button. * Add button or action for opening in web browser. Question is where... A new button?, replacing the page actions button? inside the page actions button? * We would keep always visible the back/forward buttons.
Created attachment 209823 [details] [review] Patch: browse to external links in app window This is an implementation of my proposal: * External links to same window do not open external browser anymore. * On showing in the app mode a location that's outside the app mode origin, we show the location toolbar non editable. We also show a button "open in browser". * We hide the extra menu anyway. We only show back/forward buttons, address entry and "open in browser" button. * Added to contextual menu the action "open in browser". For testing, branch "new-app-mode-links" is available in my github: https://github.com/jdapena/epiphany/tree/new-app-mode-links
I've also created 2 screencast for showing the implemented behavior in comment 8: http://youtu.be/G6xthkMKOwk http://youtu.be/fn9GvqyF8rM Setting also "usability" keyword, as we need some input from UX team.
'Open in browser'? That goes in the opposite direction of recent developments where we try not to force people to think about what type of application they use. How about 'Open as webpage'?
Created attachment 210223 [details] [review] Patch: browse to external links in app window (release 2) Updated the patch with latest changes. I've updated the label following comment 10, renamed action to "Open as Web page". Also added translation support, which I forgot. Also I used some ideas from the sketch by Felipe Erias here https://github.com/felipeerias/sketches/blob/master/epiphany/webapp_externallink.png * Added a button "Back to _app name_" to the navigation box in toolbar. So user will get always a way to go back to the application. The behavior is: if the app is close in navigation history (3 steps), then the button will go back to the last state in the application. If not, then it goes to the launch page. * Moved the "Open in Web page" action to the right, where the page related actions are usually shown. * Didn't add any change to the way the URL is shown. This can wait for the new toolbar design implementation.
Created attachment 210224 [details] Capture of the toolbar with patch to browse external links inside app window
I actually like this after trying it out, even if it felt a little too complex for something that was initially "make sure we open external links in Web". I do have a few minor notes: - I wouldn't link the 'back to app' button with the navigation. - The '<app icon(16x16px)> Name' is probably better than 'Back to Name' for the button label - The URL entry is the weakest part. While temporary in Web itself, it really feels odd to expose an uneditable URL in what is not an insensitive widget (I take it it's due to the reload button). In future we might be recoloring the toolbar so it stands out compared to the regular Web toolbar.
Created attachment 210272 [details] [review] Patch: browse to external links in app window (release 3) New iteration of the patch with recommendations in comment 13: * Now the "Back to app" button shows "<app icon> Name". * Now the back to app button is not grouped with back/forward. * Set the URL entry as insensitive.
*** Bug 672722 has been marked as a duplicate of this bug. ***
Created attachment 212256 [details] [review] Patch: browse to external links in app window (rebase 2012-04-18) Rebased the patch to latest Epiphany. It would be great that we could introduce this behavior before the expected toolbar rework. Any doubt on this proposed behavior? Any proposal to make things different?
Review of attachment 212256 [details] [review]: ::: embed/ephy-embed-shell.c @@ +279,3 @@ + if (embed_shell->priv->app_mode_origin) + g_free (embed_shell->priv->app_mode_origin); + embed_shell->priv->app_mode_origin = g_value_dup_string (value); Nitpicks, g_free is NULL-safe :) ::: src/ephy-main.c @@ +496,3 @@ + soup_uri = soup_uri_new (arguments[0]); + app_mode_origin = g_strdup (soup_uri->host); + soup_uri_free (soup_uri); It seems a bit redundant to create another parameter that is just the host of the launch URI. The shell can figure it out internally. @@ +501,3 @@ + app_mode_origin, + arguments[0], + app_name); I don't really like adding another constructor, and it does not seem that we actually need this (correct me if I'm wrong). All that data can be accessed from the .desktop file in Ephy(Embed)Shell at worst, instead of doing it in the main function. Maybe for bonus points we could have an EphyApplicationShell or something, but a simple if (application_mode) { ... } thing somewhere seems more than enough for now. Also I think we can separate this (at least) in two patches: one that does all the plumbing in main/shell/etc, and another that actually changes the UI in app mode. ::: src/ephy-navigation-history-action.c @@ +120,3 @@ + webkit_web_view_load_uri (web_view, ephy_embed_shell_get_app_mode_launch_uri (embed_shell)); + webkit_web_back_forward_list_clear (history); + } I think I'd just save the last app URI the moment you "exit app-mode", and go back to that when the user activates the action. Saves you all this code and the need of saving the launch-uri it seems. Also I think it's better to just make this a new action. ::: src/ephy-toolbar.c @@ +118,3 @@ + } else { + back_to_application_icon = gtk_image_new_from_stock ("web", GTK_ICON_SIZE_BUTTON); + } Is there any case when we actually can not have an icon? We shouldn't. @@ +120,3 @@ + } + g_free (app_icon_filename); + back_to_app = gtk_tool_button_new (back_to_application_icon, "Hola"); Hola! @@ +268,3 @@ return GTK_WIDGET (g_object_new (EPHY_TYPE_TOOLBAR, "window", window, + "toolbar-style", GTK_TOOLBAR_BOTH_HORIZ, Do you need this if you are manually creating the icon with the image? I'm not sure. ::: src/ephy-window.c @@ +170,3 @@ { "TabsDetach", NULL, N_("_Detach Tab"), NULL, NULL, G_CALLBACK (window_cmd_tabs_detach) }, + Blank line. @@ +1409,3 @@ + "window", window, + NULL); + g_signal_connect (G_OBJECT (action), No need for the G_OBJECT cast.
Created attachment 213659 [details] [review] Patch: browse to external links in app window (release 4 and rebase to 2012-05-08) Updated patch: * We don't store in EphyEmbedShell the launch uri and the title anymore. We can retrieve them from invokation and g_get_application_name. * Now we don't use a special constructor for EphyShell. Initialization of app origin happens now on processing queue_commands in EphyShell, the place where we finally process the URI arguments. * Added helper to check if an address matches the application origin ephy_embed_shell_address_in_web_app_origin. * Now we store in the web views the last web app address visited. This is used for the "back to application" button. Now we simply go to the last address in application origin. * Added new action EphyBackToWebAppAction, removing the implementation from EphyNavigationHistoryAction. * Now we catch attempts to open an address out of web app origin with target != NULL. In this case we open the URI in external browser. * Some other small fixes detected in code review.
Created attachment 218347 [details] [review] Patch: browse to external links in app window (release 5 and rebase to 2012-07-09) New release of the patch: * Detected a serious flaw on using the last app link in address, which happens to fail on going to external pages through a redirect in the origin page (i.e. in facebook). So now we use always the page history. Removed all the code to keep the latest app link in origin, as we'll now traverse the history. * Tested in wk2 branch too. It works as in wk1.
*** Bug 681341 has been marked as a duplicate of this bug. ***
*** Bug 685217 has been marked as a duplicate of this bug. ***
*** Bug 699530 has been marked as a duplicate of this bug. ***
It seems that in the gmail.com case the new links are opened in a new tab. It is possible to close the new tab with hotkeys and get back to the app. Perhaps we can interpret all new tab requests to open in a different window?
Adding myself to CC. Any update on this? Are we good to review/merge patch from comment 19?
The patch has 1 year, so I don't really know how it fits today.
Comment on attachment 197676 [details] [review] app-mode: Load pages in external hosts in a new ephy instance Doesn't apply cleanly anymore
Comment on attachment 218347 [details] [review] Patch: browse to external links in app window (release 5 and rebase to 2012-07-09) Doesn't apply cleanly anymore as well.
*** Bug 708634 has been marked as a duplicate of this bug. ***
If the only objection to the patch is that it doesn't apply anymore, why wasn't it merged in the first place? :)
*** Bug 720030 has been marked as a duplicate of this bug. ***
I'm not going to pretend I understand any of the above, but clicking a link still breaks Gmail and Google Keep at least. As a user, all I expect is for any link in any application to open in my default browser. When the app turns into a browser, I panic.
The case of gmail is a bit complicated, because it does weird things with the _blank links. Not sure how, I guess there's some javascript code, but what I see is that it opens a new window with no URL (so we can't know if it's an external link or not), and after the window is shown, it loads the actual URL. So, we could create the window, but it doesn't have a URL yet, we don't show it, and keep watching the uri of the web view (or wait until the policy decision signal is emitted for the new web view with a valid URL). Once we have a uri, if it's external we just destroy the hidden window and load the url in the default browser, otherwise we just show the window. I'll submit some patches.
Created attachment 288216 [details] [review] file-helpers: Add ephy_file_open_uri_in_default_browser()
Created attachment 288217 [details] [review] embed: Add ephy_embed_utils_urls_have_same_origin()
Created attachment 288218 [details] [review] window: Simplify the code to open external uris in default browser when in app mode
Created attachment 288219 [details] [review] app-mode: Make sure external links are openend in the default web browser
Using those last 4 patches doesn't fix the bug from comment 0, or the same problem within Google+. Contrary to the original comment though, they open in a new tab, which we don't see the top of, but can still navigate away from using ctrl+PageUp/Down.
Disabling new-windows-in-tabs makes the link work the first time it's clicked, but not afterwards.
Both netvibes and google+ work for me after applying the patch from bug 712402 on top of the last 4 here.
Review of attachment 288216 [details] [review]: ::: lib/ephy-file-helpers.c @@ +838,3 @@ + uris.next = uris.prev = NULL; + + GError *error = NULL; I think you could use g_app_info_launch_default_for_uri () instead here.
Review of attachment 288217 [details] [review]: ::: embed/ephy-embed-utils.c @@ +316,3 @@ + b_uri = soup_uri_new (b_url); + if (b_uri) { + I'm wondering whether soup_uri_host_equal () should be used here instead (port at least might be relevant).
Review of attachment 288218 [details] [review]: Good.
(In reply to comment #40) > Review of attachment 288216 [details] [review]: > > ::: lib/ephy-file-helpers.c > @@ +838,3 @@ > + uris.next = uris.prev = NULL; > + > + GError *error = NULL; > > I think you could use g_app_info_launch_default_for_uri () instead here. We don't need to guess anything based on the URI, we know we want the default web browser.
(In reply to comment #41) > Review of attachment 288217 [details] [review]: > > ::: embed/ephy-embed-utils.c > @@ +316,3 @@ > + b_uri = soup_uri_new (b_url); > + if (b_uri) { > + > > I'm wondering whether soup_uri_host_equal () should be used here instead (port > at least might be relevant). Indeed, I didn't know soup_uri_host_equal(), I'll use it.
Review of attachment 288219 [details] [review]: Looks good, didn't test though. ::: src/ephy-window.c @@ +2052,3 @@ + */ + g_object_set_data_full (G_OBJECT (window), "referrer", + g_strdup (webkit_web_view_get_uri(parent_web_view)), Nit: space after function name.
This should be fixed now