Issue 1592 - Fixed frame collapsing on Safari

safari/ext/content.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 48 matching lines @@
   {
     var url = resolveURL(event.url);
 
     // we don't block non-HTTP requests anyway, so we can bail out
     // without asking the background page. This is even necessary
     // because passing large data (like a photo encoded as data: URL)
     // to the background page, freezes Safari.
     if (!/^https?:/.test(url))
       return;
 
-    var type;
+    var type = "other";
+    var eventName = "error";
+
     switch(event.target.localName)
     {
       case "frame":
       case "iframe":
         type = "sub_frame";
+        eventName = "load";
         break;
       case "img":
         type = "image";
         break;
       case "object":
       case "embed":
         type = "object";
         break;
       case "script":
         type = "script";
         break;
       case "link":
         if (/\bstylesheet\b/i.test(event.target.rel))
-        {
           type = "stylesheet";
-          break;
-        }
-      default:

[Sebastian Noack, 2014/11/22 13:53:46]

Sorry for this unrelated change. But now, where the default eventName is
initialized above the the switch/case construct, it's more consistent and
readable to initialize the "type" variable in the same way, IMO.

[kzar, 2014/11/24 16:54:50]

Makes sense to me.

-        type = "other";
+        break;
     }
 
     if (!safari.self.tab.canLoad(
       event, {
         category: "webRequest",
         url: url,
         type: type,
         pageId: documentInfo.pageId,
         frameId: documentInfo.frameId
       }
     ))
     {
       event.preventDefault();
 
       // Safari doesn't dispatch an "error" event when preventing an element
-      // from loading by cancelling the "beforeload" event. So we have to
-      // dispatch it manually. Otherwise element collapsing wouldn't work.
-      if (type != "sub_frame")
+      // from loading by cancelling the "beforeload" event. Starting with

[kzar, 2014/11/24 16:54:50]

Nitpick: This comment doesn't read very well starting at the second sentence.
Maybe rewrite?

[Sebastian Noack, 2014/11/24 17:13:20]

Can you explain why it doesn't read well in your opinion? How would you improve
it?

[kzar, 2014/11/25 11:27:33]

The part "...it not even dispatches..." would be more idiomatically written as
"...it doesn't even dispatch..." but either fragment doesn't fit in the sentence
that well. How about this?

"Safari doesn't dispatch all the expected events for elements that have been
prevented from loading by having their "beforeload" event cancelled. "error"
events are never dispatched and with versions of Safari 8 and up "load" events
are also not dispatched. We need to dispatch those events manually here to avoid
breaking element collapsing and therefore the pages that rely on it."

[Sebastian Noack, 2014/11/25 11:48:38]

I used a sligthly modified version of your text. I changed following:

1. I prefer to explicitly mentioning the relationship between "load" events and
frames.
2. Some pages are not only broken in the sense that element collapsing won't
work, but might also break in a much more fatal way, when they rely on those
events being dispatched. I remember an issue with a website which weren't
functional until their "load" or "error" event listener were called.

[kzar, 2014/11/25 12:06:53]

Cool, reads great

+      // Safari 8, it not even dispatches a "load" event for cancelled frames.
+      // So we have to dispatch it manually. Otherwise, element collapsing
+      // wouldn't work, and pages that rely on those events break.
+      if (type != "sub_frame" || parseInt(navigator.userAgent.match(/\bVersion\/(\d+)/)[1], 10) >= 8)

[kzar, 2014/11/24 16:54:50]

What if the user is using a version < 8, shouldn't the error event still be
dispatched?

[Sebastian Noack, 2014/11/24 17:13:20]

Yes, and it is. However, error events should only be dispatched for elements
other than frames, in which case the check passes.

[kzar, 2014/11/25 11:27:33]

Ah I see, sorry.

       {
         setTimeout(function()
         {
           var evt = document.createEvent("Event");
-          evt.initEvent("error");
+          evt.initEvent(eventName);
           event.target.dispatchEvent(evt);
         }, 0);
       }
     }
   }, true);
 
 
   /* Context menus */
 
   document.addEventListener("contextmenu", function(event)
@@ skipping 297 matching lines @@
             messageProxy.handleResponse(event.message);
             break;
           case "proxyCallback":
             backgroundPageProxy.handleCallback(event.message);
             break;
         }
       }
     }
   });
 })();