GNOME Bugzilla – Bug 607233
Don't open empty window when pdf file with target="_blank" link is downloaded
Last modified: 2013-12-10 10:09:29 UTC
When clicking on a link to a PDF file with target="_blank", Epiphany opens a new empty window and shows a download/open dialog. It should not open a useless empty window, just like Firefox do. This can be seen for example on http://www.vrt.be with the pdf files on the right side.
Created attachment 207164 [details] [review] ephy-embed: close tabs for target=_blank downloads Many web sites use target="_blank" for their download links. This creates a lot of useless tabs/windows in Epiphany. Request to close any WebView that is created in such scenario.
Review of attachment 207164 [details] [review]: Oh yes, please =) will this create the window and then make it disappear? Would it be better to just wait until you know whether it's just a download or not?
(In reply to comment #2) > Review of attachment 207164 [details] [review]: > > Oh yes, please =) will this create the window and then make it disappear? Would > it be better to just wait until you know whether it's just a download or not? Talking off the top of my head, but I think the moment when we create new tabs/windows happens earlier than the moment when we know if it is a download? If not I agree we should do what Gustavo says.
Created attachment 211522 [details] [review] ephy-embed: close tabs for target=_blank downloads Updated approach. This time I am adding GObject data: last-target and parent-last-target. In ephy-window, on policy-requests, we store the value of webkit_web_navigation_action_get_target_frame as last-target. Then when a new web-view is requested in create_web_view_cb we transfer this value to the child WebKitWebView as "parent-last-target". Then in ephy-embed, on download_requested_cb, we check if this value is _blank, and the view has no URI set, and the load state is PROVISIONAL. This seems to be safe, I would like opinions on the approach. I will test this in my installed Epiphany.
Created attachment 211525 [details] [review] ephy-shell: close tabs for target=_blank downloads Many web sites use target="_blank" for their download links. This creates a lot of useless tabs/windows in Epiphany. Delay the embedding of new tabs in ephy-shell until they are out of suspicion of being one of this _blank tabs. -- This is an alternate version, as suggested by Martin instead of destroying, we simply don't embed the view until we are sure it is useful. It has some quirks though. Since last-target is only set after policy_decision, a window with a NULL last-target that uses the popup-menu to Open in new window/new tab, will see new blank tabs or windows. I have not come up with a way to have a 100% reliable last-target. I did not consider DOM stuff because Xan said it is going away, plus it is not 100% guaranteed either (think this javascript "/#!/link" ridden applications). I think this approach is a bit less flawed than the previous one. But both are possible paths. I also considered adding the last-target property to EphyWebView, I have this same patch using such property. Not a big change, just perhaps tidier.
Created attachment 215110 [details] [review] ephy-web-view: avoid target=_blank download views Many web sites use target="_blank" for their download links. Keep an eye on WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT and Content-Disposition "attachment" to know if a EphyWebView has an empty layout and was meant for download. In such cases fire a signal "empty-download-view" handled by EphyWindow. EphyWindow handles this by requesting the close of the EphyWebView and presenting the previous window to the user (otherwise no window has focus when the /new/ window is destroyed). EphyWindow had the relevant changes to handle downloads properly. Patch needs more testing, but could already use a light review. Also, I believe it can be done without a new signal, by just calling close-web-view in EphyWebView and handling the gtk_window_present bit in EphyWindow, with g_signal_connect_after (view, "close-web-view"). Requesting closes in EphyWebView feels a bit out of place, though. Also, consider "target-blank" GObject data: this is a leftover from a previous patch, but I am a bit unsure if removing this check would be too risky. The combination of empty-layout + content-disposition seems to be solid so far, this should be removable. Your opinion? :-)
OK, so: - I feel a bit uncomfortable doing this with what we perceive as heuristics. We should either be sure it's safe to close the view, or not do it at all. For what is worth, Firefox in its latest version does not close empty views that trigger downloads, so either this is not trivial to get right or they just didn't bother (worth investigating; also probably worth reading Chrome's code, assuming they do this). - All that being said, I cannot imagine how closing an empty view that triggers a download could be unsafe, so in that regard the rule seems sane. - As for the target=_blank stuff, it seems there's actually a whole lot of target frame options, so this is not comprehensive; http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#valid-browsing-context-name-or-keyword - Last, am I reading the patch wrong or are you emitting the signal in the LOAD_FAILED case? Does a load fail when a download starts?
*** Bug 598584 has been marked as a duplicate of this bug. ***
*** Bug 114949 has been marked as a duplicate of this bug. ***
Created attachment 231257 [details] [review] ephy-web-view: avoid target=_blank download views Many web sites use target="_blank" for their download links. Keep an eye on WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT and Content-Disposition "attachment" to know if a EphyWebView has an empty layout and therefor an "empty download". If that is the case, request closing the view. EphyWindow makes sure that we are not closing the last tab of the session. ---- This is a much simpler approach that so far works better than the previous one. YEY! The behavior is the same as in chrome: ctrl+click or target=_blank downloads tabs are closed as soon as they trigger the download. The download widget is always in a visible window. I have a warning caused somehow by a late dispatchDidFailLoading(). I will add a comment separately with the trace, since I have no clue if this is our fault or WebKit's.
Here's the trace. It is not a crash, it's only a critical warning. Happens every time the code automagically closes the empty download tab. (epiphany:27531): GLib-CRITICAL **: g_hash_table_lookup_extended: assertion `hash_table != NULL' failed Program received signal SIGTRAP, Trace/breakpoint trap. g_logv (log_domain=log_domain@entry=0xb55f650e "GLib", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0xb55fea36 "%s: assertion `%s' failed", args=args@entry=0xbfffe1bc "-\247_\265/\245_\265\066") at gmessages.c:978 978 g_private_set (&g_log_depth, GUINT_TO_POINTER (depth)); (gdb) bt
+ Trace 231283
Created attachment 251283 [details] [review] If a tab is created only for a download, close it If a tab that has just been created ends up being just a download, then close it down. If it's the only one on the window, then make it show the overview instead.
I hadn't seen this bug, so I went and duplicated some of the work I guess =X. My approach is a bit more conservative, I think untying downloads from specific windows would allow a more complete solution which also avoided empty new windows (my patch only handles existing windows with new tabs opened just for the file). Any idea in which file should I shove a test for this?
Review of attachment 251283 [details] [review]: good!
Comment on attachment 251283 [details] [review] If a tab is created only for a download, close it 29c990d5a4662467e4708db1ab1cc06c4901fc7f
Thanks!