Issue 6549206625943552: Issue 1592 - Fixed frame collapsing on Safari (Closed)
|
Created:
Nov. 22, 2014, 1:48 p.m. by Sebastian Noack Modified:
Nov. 25, 2014, 4:33 p.m. Visibility:
Public. |
DescriptionIssue 1592 - Fixed frame collapsing in Safari 8
Patch Set 1 #
Total comments: 10
Patch Set 2 : Improved comment #Patch Set 3 : Apparently this behaviour isn't limited to Safari 8 #MessagesTotal messages: 8
http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (left): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:92: default: 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.
http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (left): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:92: default: On 2014/11/22 13:53:46, Sebastian Noack wrote: > 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. Makes sense to me. http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (right): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:108: // from loading by cancelling the "beforeload" event. Starting with Nitpick: This comment doesn't read very well starting at the second sentence. Maybe rewrite? http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:112: if (type != "sub_frame" || parseInt(navigator.userAgent.match(/\bVersion\/(\d+)/)[1], 10) >= 8) What if the user is using a version < 8, shouldn't the error event still be dispatched?
http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (right): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:108: // from loading by cancelling the "beforeload" event. Starting with On 2014/11/24 16:54:50, kzar wrote: > Nitpick: This comment doesn't read very well starting at the second sentence. > Maybe rewrite? Can you explain why it doesn't read well in your opinion? How would you improve it? http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:112: if (type != "sub_frame" || parseInt(navigator.userAgent.match(/\bVersion\/(\d+)/)[1], 10) >= 8) On 2014/11/24 16:54:50, kzar wrote: > What if the user is using a version < 8, shouldn't the error event still be > dispatched? Yes, and it is. However, error events should only be dispatched for elements other than frames, in which case the check passes.
http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (right): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:108: // from loading by cancelling the "beforeload" event. Starting with On 2014/11/24 17:13:20, Sebastian Noack wrote: > On 2014/11/24 16:54:50, kzar wrote: > > Nitpick: This comment doesn't read very well starting at the second sentence. > > Maybe rewrite? > > Can you explain why it doesn't read well in your opinion? How would you improve > it? 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." http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:112: if (type != "sub_frame" || parseInt(navigator.userAgent.match(/\bVersion\/(\d+)/)[1], 10) >= 8) On 2014/11/24 17:13:20, Sebastian Noack wrote: > On 2014/11/24 16:54:50, kzar wrote: > > What if the user is using a version < 8, shouldn't the error event still be > > dispatched? > > Yes, and it is. However, error events should only be dispatched for elements > other than frames, in which case the check passes. Ah I see, sorry.
http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (right): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:108: // from loading by cancelling the "beforeload" event. Starting with On 2014/11/25 11:27:33, kzar wrote: > On 2014/11/24 17:13:20, Sebastian Noack wrote: > > On 2014/11/24 16:54:50, kzar wrote: > > > Nitpick: This comment doesn't read very well starting at the second > sentence. > > > Maybe rewrite? > > > > Can you explain why it doesn't read well in your opinion? How would you > improve > > it? > > 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." 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.
LGTM http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... File safari/ext/content.js (right): http://codereview.adblockplus.org/6549206625943552/diff/5629499534213120/safa... safari/ext/content.js:108: // from loading by cancelling the "beforeload" event. Starting with On 2014/11/25 11:48:38, Sebastian Noack wrote: > On 2014/11/25 11:27:33, kzar wrote: > > On 2014/11/24 17:13:20, Sebastian Noack wrote: > > > On 2014/11/24 16:54:50, kzar wrote: > > > > Nitpick: This comment doesn't read very well starting at the second > > sentence. > > > > Maybe rewrite? > > > > > > Can you explain why it doesn't read well in your opinion? How would you > > improve > > > it? > > > > 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." > > 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. Cool, reads great
LGTM 
|
