Issue 1592 - Fixed frame collapsing on Safari

safari/ext/content.js

Index: safari/ext/content.js
===================================================================
--- a/safari/ext/content.js
+++ b/safari/ext/content.js
@@ -66,12 +66,15 @@
     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";
@@ -85,12 +88,8 @@
         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(
@@ -106,14 +105,16 @@
       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);
       }