After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 658395 - Escaped from "Application mode"
Escaped from "Application mode"
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Web Applications
git master
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
: 666362 672722 681341 685217 699530 708634 720030 (view as bug list)
Depends on:
Blocks: 669606
 
 
Reported: 2011-09-06 20:46 UTC by Bastien Nocera
Modified: 2014-10-14 13:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
example (429 bytes, text/html)
2011-09-13 09:32 UTC, Claudio Saavedra
  Details
app-mode: Load pages in external hosts in a new ephy instance (3.45 KB, patch)
2011-09-28 14:49 UTC, Claudio Saavedra
needs-work Details | Review
Patch: browse to external links in app window (19.40 KB, patch)
2012-03-15 12:47 UTC, Jose Dapena Paz
none Details | Review
Patch: browse to external links in app window (release 2) (28.82 KB, patch)
2012-03-21 09:38 UTC, Jose Dapena Paz
none Details | Review
Capture of the toolbar with patch to browse external links inside app window (10.67 KB, image/png)
2012-03-21 09:40 UTC, Jose Dapena Paz
  Details
Patch: browse to external links in app window (release 3) (31.00 KB, patch)
2012-03-21 19:07 UTC, Jose Dapena Paz
none Details | Review
Patch: browse to external links in app window (rebase 2012-04-18) (29.77 KB, patch)
2012-04-18 07:20 UTC, Jose Dapena Paz
reviewed Details | Review
Patch: browse to external links in app window (release 4 and rebase to 2012-05-08) (37.64 KB, patch)
2012-05-08 11:33 UTC, Jose Dapena Paz
none Details | Review
Patch: browse to external links in app window (release 5 and rebase to 2012-07-09) (36.41 KB, patch)
2012-07-09 15:52 UTC, Jose Dapena Paz
needs-work Details | Review
file-helpers: Add ephy_file_open_uri_in_default_browser() (2.75 KB, patch)
2014-10-10 13:00 UTC, Carlos Garcia Campos
committed Details | Review
embed: Add ephy_embed_utils_urls_have_same_origin() (1.97 KB, patch)
2014-10-10 13:00 UTC, Carlos Garcia Campos
committed Details | Review
window: Simplify the code to open external uris in default browser when in app mode (2.23 KB, patch)
2014-10-10 13:02 UTC, Carlos Garcia Campos
committed Details | Review
app-mode: Make sure external links are openend in the default web browser (3.12 KB, patch)
2014-10-10 13:03 UTC, Carlos Garcia Campos
committed Details | Review

Description Bastien Nocera 2011-09-06 20:46:46 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.
Comment 1 Bastien Nocera 2011-09-06 20:46:56 UTC
epiphany-3.1.90-1.fc16.x86_64
Comment 2 Claudio Saavedra 2011-09-13 09:32:25 UTC
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.
Comment 3 Claudio Saavedra 2011-09-28 14:49:30 UTC
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
Comment 4 Claudio Saavedra 2011-09-28 14:54:16 UTC
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.
Comment 5 Claudio Saavedra 2011-09-28 14:57:10 UTC
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.
Comment 6 Xan Lopez 2012-03-14 12:55:48 UTC
*** Bug 666362 has been marked as a duplicate of this bug. ***
Comment 7 Jose Dapena Paz 2012-03-15 12:29:50 UTC
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.
Comment 8 Jose Dapena Paz 2012-03-15 12:47:47 UTC
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
Comment 9 Jose Dapena Paz 2012-03-16 16:19:27 UTC
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.
Comment 10 Reinout van Schouwen 2012-03-19 22:56:05 UTC
'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'?
Comment 11 Jose Dapena Paz 2012-03-21 09:38:32 UTC
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.
Comment 12 Jose Dapena Paz 2012-03-21 09:40:12 UTC
Created attachment 210224 [details]
Capture of the toolbar with patch to browse external links inside app window
Comment 13 Jakub Steiner 2012-03-21 12:37:31 UTC
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.
Comment 14 Jose Dapena Paz 2012-03-21 19:07:04 UTC
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.
Comment 15 Diego Escalante Urrelo (not reading bugmail) 2012-03-26 18:21:20 UTC
*** Bug 672722 has been marked as a duplicate of this bug. ***
Comment 16 Jose Dapena Paz 2012-04-18 07:20:26 UTC
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?
Comment 17 Xan Lopez 2012-04-25 16:53:13 UTC
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.
Comment 18 Jose Dapena Paz 2012-05-08 11:33:38 UTC
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.
Comment 19 Jose Dapena Paz 2012-07-09 15:52:25 UTC
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.
Comment 20 Xan Lopez 2012-12-08 18:02:11 UTC
*** Bug 681341 has been marked as a duplicate of this bug. ***
Comment 21 Xan Lopez 2012-12-08 18:03:03 UTC
*** Bug 685217 has been marked as a duplicate of this bug. ***
Comment 22 Jakub Steiner 2013-05-03 13:41:11 UTC
*** Bug 699530 has been marked as a duplicate of this bug. ***
Comment 23 William Jon McCann 2013-05-04 20:54:56 UTC
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?
Comment 24 Eduardo Lima Mitev 2013-07-29 13:48:09 UTC
Adding myself to CC.

Any update on this? Are we good to review/merge patch from comment 19?
Comment 25 Jose Dapena Paz 2013-07-29 23:30:57 UTC
The patch has 1 year, so I don't really know how it fits today.
Comment 26 Bastien Nocera 2013-07-30 13:27:42 UTC
Comment on attachment 197676 [details] [review]
app-mode: Load pages in external hosts in a new ephy instance

Doesn't apply cleanly anymore
Comment 27 Bastien Nocera 2013-07-30 13:28:19 UTC
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.
Comment 28 Claudio Saavedra 2013-09-26 17:07:00 UTC
*** Bug 708634 has been marked as a duplicate of this bug. ***
Comment 29 Simon Lindgren 2013-09-30 12:20:59 UTC
If the only objection to the patch is that it doesn't apply anymore, why wasn't it merged in the first place? :)
Comment 30 Claudio Saavedra 2013-12-08 12:47:12 UTC
*** Bug 720030 has been marked as a duplicate of this bug. ***
Comment 31 Juha Siltala 2014-07-15 17:06:02 UTC
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.
Comment 32 Carlos Garcia Campos 2014-10-10 12:55:07 UTC
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.
Comment 33 Carlos Garcia Campos 2014-10-10 13:00:16 UTC
Created attachment 288216 [details] [review]
file-helpers: Add ephy_file_open_uri_in_default_browser()
Comment 34 Carlos Garcia Campos 2014-10-10 13:00:48 UTC
Created attachment 288217 [details] [review]
embed: Add ephy_embed_utils_urls_have_same_origin()
Comment 35 Carlos Garcia Campos 2014-10-10 13:02:20 UTC
Created attachment 288218 [details] [review]
window: Simplify the code to open external uris in default browser when in app mode
Comment 36 Carlos Garcia Campos 2014-10-10 13:03:00 UTC
Created attachment 288219 [details] [review]
app-mode: Make sure external links are openend in the default web browser
Comment 37 Bastien Nocera 2014-10-13 15:14:26 UTC
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.
Comment 38 Bastien Nocera 2014-10-13 15:31:53 UTC
Disabling new-windows-in-tabs makes the link work the first time it's clicked, but not afterwards.
Comment 39 Bastien Nocera 2014-10-13 15:39:16 UTC
Both netvibes and google+ work for me after applying the patch from bug 712402 on top of the last 4 here.
Comment 40 Claudio Saavedra 2014-10-14 11:02:19 UTC
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.
Comment 41 Claudio Saavedra 2014-10-14 11:07:22 UTC
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).
Comment 42 Claudio Saavedra 2014-10-14 11:08:05 UTC
Review of attachment 288218 [details] [review]:

Good.
Comment 43 Carlos Garcia Campos 2014-10-14 11:30:18 UTC
(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.
Comment 44 Carlos Garcia Campos 2014-10-14 11:32:20 UTC
(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.
Comment 45 Claudio Saavedra 2014-10-14 13:10:00 UTC
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.
Comment 46 Carlos Garcia Campos 2014-10-14 13:23:45 UTC
This should be fixed now