So the plan was to add beforeunload support to WebKitGTK+, but disable it in Epiphany to prevent sites from popping up those "are you sure you want to leave?" dialogs. If sites are using it responsibly then maybe we should reconsider.

(In reply to Michael Catanzaro from comment #1)
> So the plan was to add beforeunload support to WebKitGTK+, but disable it in
> Epiphany to prevent sites from popping up those "are you sure you want to
> leave?" dialogs. If sites are using it responsibly then maybe we should
> reconsider.

Disabling it in epiphany *by default*, but making it available to users with a gsetting. That's still my plan, but the WebKit patch is still waiting a review.

Based on complaints from users about data loss, I think we should enable this by default. It's how other browsers work, so it can't be too bad.

We might need the "prevent site from opening additional alerts" checkbox on our alerts, though.

Created attachment 319051 [details] [review]
Run onbeforeunload when closing tabs

WebKitGTK+ 2.11.3 adds support for onbeforeunload, and calls it
automatically when navigating from one page to another in a web view.
But it is the application's responsibility to handle this when closing
web views, using webkit_web_view_try_close().

Note that we do not run onbeforeunload when closing the entire window.
To do this properly, we need to switch to each web view when displaying
the JS dialog for it, but this is not currently possible with our API.

Review of attachment 319051 [details] [review]:

I don't understand the commit message, what is not currently possible with our API? We do that for modified forms, we check the web views, and jump to the first one that has modified forms to show the confirm dialog. It's not perfect, though, because if there's more than one we only check the first one, but fortunately that case is not that common. I think when beforeunload is really useful (if it's useful at all) is for application mode, and in that case we really need to do this when the application is closed. Actually I would add a gsetting to disable beforeunlod by default in browser and mode and enable it in web app mode. I think we should do this the same way than modified forms. What's the reasoning to remove the notebook signal and move the code in ephy-window? Is that needed for this patch or just a refactoring?

I gave the example of the flickr uploader in my original report. No forms there. Yet unbeforeunload allows them to warn users.

(In reply to Carlos Garcia Campos from comment #5)
> I don't understand the commit message, what is not currently possible with
> our API?

The problem is that if a tab displays a dialog, we need to switch to that tab. After calling webkit_web_view_try_close(), the web view will emit "close" if there is no dialog or if the user clicks through the dialog, and it will not emit anything otherwise, so how can Epiphany know to switch to that tab?

I see now we have a "script-dialog" signal. I wonder if that is emitted in this case; if so, it should be possible to implement.

> We do that for modified forms, we check the web views, and jump to
> the first one that has modified forms to show the confirm dialog. It's not
> perfect, though, because if there's more than one we only check the first
> one, but fortunately that case is not that common. I think when beforeunload
> is really useful (if it's useful at all) is for application mode, and in
> that case we really need to do this when the application is closed.

Yes, I agree, I should try reworking the patch to call onbeforeload when closing the window.

> Actually
> I would add a gsetting to disable beforeunlod by default in browser and mode
> and enable it in web app mode.

I agree with adding a gsetting. I think I would rather have it on by default, following the rule "do what every other major browser does," but there's value in either approach....

> I think we should do this the same way than
> modified forms. What's the reasoning to remove the notebook signal and move
> the code in ephy-window? Is that needed for this patch or just a refactoring?

I've found this quite hard to explain; the basic problem is that we need to avoid creating a recursive loop between notebook_page_close_request_cb() and impl_remove_child(), and the eaisest way is to just remove the signal and use try_close() as the new way to handle close requests.

There needs to be some way to tell EphyWindow to consider closing the tab (it checks for modified forms, and for active downloads if it is the last active tab, to decide whether to actually close the tab). Currently that is done via EphyNotebook's "tab-close-request" signal. This signal is emitted by the notebook when you click a close button on a tab, or by window commands when clicking the close menu item, or by EphyWindow itself in response to the "close" signal of WebKitWebView (when "close" is emitted, EphyWebView calls ephy_embed_container_remove_child(), the implementation of which is impl_remove_child() in EphyWindow, which emits the EphyNotebook's "tab-close-request" signal).

It's independently nice to refactor this, since it's awkward for the tab close request to unnecessarily go through EphyNotebook for no reason in two of the three cases above. But anyway, we just need some way to ensure that try_close() is called at most once, and that the code currently in notebook_page_close_request_cb() runs exactly once. The easiest way is to replace "tab-close-request" with try_close().

(In reply to Emmanuel Touzery from comment #6)
> I gave the example of the flickr uploader in my original report. No forms
> there. Yet unbeforeunload allows them to warn users.

Yeah, the commit message perhaps makes it seem like it's desirable to not call onbeforeonload when closing the window, but it's of course not; it's just harder to implement for closing the window than for closing a tab. Mentioning the modified form checker was basically just a bad excuse for my unsatisfactory code. :p

(In reply to Michael Catanzaro from comment #7)
> (In reply to Carlos Garcia Campos from comment #5)
> > I don't understand the commit message, what is not currently possible with
> > our API?
> 
> The problem is that if a tab displays a dialog, we need to switch to that
> tab. After calling webkit_web_view_try_close(), the web view will emit
> "close" if there is no dialog or if the user clicks through the dialog, and
> it will not emit anything otherwise, so how can Epiphany know to switch to
> that tab?
> 
> I see now we have a "script-dialog" signal. I wonder if that is emitted in
> this case; if so, it should be possible to implement.

Yes, we could connect to script-dialog.

> > We do that for modified forms, we check the web views, and jump to
> > the first one that has modified forms to show the confirm dialog. It's not
> > perfect, though, because if there's more than one we only check the first
> > one, but fortunately that case is not that common. I think when beforeunload
> > is really useful (if it's useful at all) is for application mode, and in
> > that case we really need to do this when the application is closed.
> 
> Yes, I agree, I should try reworking the patch to call onbeforeload when
> closing the window.
> 
> > Actually
> > I would add a gsetting to disable beforeunlod by default in browser and mode
> > and enable it in web app mode.
> 
> I agree with adding a gsetting. I think I would rather have it on by
> default, following the rule "do what every other major browser does," but
> there's value in either approach....

I don't think we should follow that rule to enable an annoying feature. 

> > I think we should do this the same way than
> > modified forms. What's the reasoning to remove the notebook signal and move
> > the code in ephy-window? Is that needed for this patch or just a refactoring?
> 
> I've found this quite hard to explain; the basic problem is that we need to
> avoid creating a recursive loop between notebook_page_close_request_cb() and
> impl_remove_child(), and the eaisest way is to just remove the signal and
> use try_close() as the new way to handle close requests.
> 
> There needs to be some way to tell EphyWindow to consider closing the tab
> (it checks for modified forms, and for active downloads if it is the last
> active tab, to decide whether to actually close the tab). Currently that is
> done via EphyNotebook's "tab-close-request" signal. This signal is emitted
> by the notebook when you click a close button on a tab, or by window
> commands when clicking the close menu item, or by EphyWindow itself in
> response to the "close" signal of WebKitWebView (when "close" is emitted,
> EphyWebView calls ephy_embed_container_remove_child(), the implementation of
> which is impl_remove_child() in EphyWindow, which emits the EphyNotebook's
> "tab-close-request" signal).
> 
> It's independently nice to refactor this, since it's awkward for the tab
> close request to unnecessarily go through EphyNotebook for no reason in two
> of the three cases above. But anyway, we just need some way to ensure that
> try_close() is called at most once, and that the code currently in
> notebook_page_close_request_cb() runs exactly once. The easiest way is to
> replace "tab-close-request" with try_close().

I think we should indeed rework the termination process before adding more conditions like this. The current approach has several problems.

 - In case of several tabs with modified data we only check the first one, if you see the data is not important and decide to continue with the close, the other tabs will not be checked.

 - If you have more than one window, and there's something in the second window that prevents the close from happening, the first window has already been closed when you cancel the close. You could get the other window back by restoring tabs, but that's a workaround and it's limited to the close tabs cache limit.

 - If you cancel the close for whatever reason, further changes will not be saved in the session, because it will be closed. We can't re-enable the session for the same reason of the previous issue, we could have closed another window already and that will be lost from history if we save it again.

So, in the end the problem is that we try to close every window and every tab individually, so when something is cancelled there's no way to go back to the previous state. It would be better if we could somehow check all the state and then force close everything in one step. I don't know if that's actually possible with beforeunload events, though. 

> (In reply to Emmanuel Touzery from comment #6)
> > I gave the example of the flickr uploader in my original report. No forms
> > there. Yet unbeforeunload allows them to warn users.
> 
> Yeah, the commit message perhaps makes it seem like it's desirable to not
> call onbeforeonload when closing the window, but it's of course not; it's
> just harder to implement for closing the window than for closing a tab.
> Mentioning the modified form checker was basically just a bad excuse for my
> unsatisfactory code. :p

:-)

This isn't looking like a quick project anymore. :(

(In reply to Carlos Garcia Campos from comment #8)
> I don't think we should follow that rule to enable an annoying feature. 

The other problem is: it's *already* enabled (as of 2.11.3), but only for navigations away from the current page inside the same web view, and it's not possible for Epiphany to disable this. If it were possible for Epiphany to disable that behavior of WebKit, then it would make sense for Epiphany-level beforeunload support to be conditional on a preference.

(In reply to Michael Catanzaro from comment #9)
> This isn't looking like a quick project anymore. :(

It never was! that's why I proposed you to simply disable beforeunload events for now.

> (In reply to Carlos Garcia Campos from comment #8)
> > I don't think we should follow that rule to enable an annoying feature. 
> 
> The other problem is: it's *already* enabled (as of 2.11.3), but only for
> navigations away from the current page inside the same web view, and it's
> not possible for Epiphany to disable this. If it were possible for Epiphany
> to disable that behavior of WebKit, then it would make sense for
> Epiphany-level beforeunload support to be conditional on a preference.

Of course it is possible!
https://git.gnome.org/browse/epiphany/commit/?id=08eb12b3bb538f96b23bc2ac7bf21e360b085b4d

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/220.