overlay-child.js contains some code that checks for existance of Drupal.onBeforeUnload? I finally found out that this is a leftover from the modalframe.module using the onbeforeunload.module.

I don't think core should such explicit call contrib modules. And besides, its unclear to me what usecase the current implementation has.

    // Install onBeforeUnload callback, if module is present.
    if ($.isObject(Drupal.onBeforeUnload) && !Drupal.onBeforeUnload.callbackExists('overlayChild')) {
      Drupal.onBeforeUnload.addCallback('overlayChild', function () {
        // Tell the parent window we're unloading.
        parent.Drupal.overlay.unbindChild(window);
      });
    }
CommentFileSizeAuthor
#1 overlayremoveunbindchild.patch2 KBcasey

Comments

casey’s picture

casey commented
Status: Active » Needs review
StatusFileSize
new overlayremoveunbindchild.patch2 KB

Ok I don't think we need it. If some module wants to use the onunload event it could easily use the onOverlayCanClose() callback to prevent closing the overlay before onunload is ready.

dries’s picture

dries commented
Status: Needs review » Reviewed & tested by the community

Less is always more. Let's mark this RTBC.

markus_petrux’s picture

markus_petrux commented

This unbindChild stuff comes from the original Modal Frame API. Here, I would like to point out that, IMO, it is not critical to remove this stuff, however it provides a better user experience because it helps control what happens when the child document is being unloaded.

I can see you don't like this stuff related to onBeforeUnload event, however there might be an alternative: The pagehide event. However, you may still need both for cross-browser compatibility issues. AFAICT, pagehide is Mozilla stuff (since Firefox 1.5), also implemented in WebKit, but I'm not sure who else.

For further information, please see: #669436: In search for alternatives to the unload event

For core, I would suggest to not remove unbindChild(), but provide a better alternative if you don't like how it is. Still, it is not absolutely critical.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

At the very least this sounds like a "needs review" to me. :)

casey’s picture

casey commented
Status: Needs review » Reviewed & tested by the community

It works fine without it. I don't think there even a user experience drawback. We shouldn't add contrib dependant code to core. Contrib modules can easily register an onunload handler by their own.

back to RTBC.

markus_petrux’s picture

markus_petrux commented

Fine by me. Just one comment to your suggestion: a page with an onunload handler is not cached on the browser.

http://webkit.org/blog/516/webkit-page-cache-ii-the-unload-event/

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.