[WebProfilerBundle] Fixes event listener attaching error in IE #13636

aik099 · 2015-02-10T10:14:32Z

I haven't tested the change, because I don't any working Symfony installation at hand. By the looks of changes it should work.

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	not sure (do we have JS tests?)
Fixed tickets	#13447
License	MIT
Doc PR

pyrech · 2015-02-10T10:26:35Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

@@ -67,6 +67,14 @@
                localStorage.setItem(profilerStorageKey + name, value);
            },

+            addEvent = function (element, eventName, callback) {
+                if (document.all) {


Maybe better to check the existence of element.addEventListener, don't you think?

There's also a drawback, in ie8 the reference to this in the event handler refers to the document object instead of the caller object. So eventHandlers using this should have an alternative to the caller object

@aik099 could you do this check around the function definition instead of doing it at runtime to do it only once ? And checking for document.addEventListener would indeed be better than document.all

could you do this check around the function definition instead of doing it at runtime to do it only once

@stof, what do you mean? If I'll be checking for existence of element.addEventListener then I can't do this outside the addEvent function

There's also a drawback, in ie8 the reference to this in the event handler refers to the document object instead of the caller object. So eventHandlers using this should have an alternative to the caller object

@wvdweij , what do you mean? I haven't changed this reference inside the callback if you're referring to it. And in JavaScript the this can be changed dynamically when needed and it's up to callback itself to preserve it.

@aik099 This is why I suggested checking on document rather than element (document is available at definition time). The benefit is that you choose the right implementation of the function only once instead of doing the check for each call

Using the method .attachEvent() in IE, In normal browsers, using .addEventListener, the var this points to the element, while in IE it points to the window object.

So using this on the button events will probably break.

Using the method .attachEvent() in IE, In normal browsers, using .addEventListener, the var this points to the element, while in IE it points to the window object.

@wvdweij , I've pushed changes for this usage. Now we have lots of boilerplate code for cross-browser event handling 😄

This is why I suggested checking on document rather than element (document is available at definition time). The benefit is that you choose the right implementation of the function only once instead of doing the check for each call

@stof , done.

stof · 2015-02-10T11:20:15Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/form.html.twig

+                        }
+
+                        if (e.target) {
+                            target = e.target;


you are missing the variable delcaration for this variable in the function. It should be a local variable

Fixed in ecc30cc

wouterj · 2015-02-10T12:49:27Z

Is there a reason not to use the common polyfill? That would catch more edge cases

aik099 · 2015-02-10T13:42:20Z

Is there a reason not to use the common polyfill? That would catch more edge cases

I have no idea, why we're using plain JS here. I guess jQuery isn't used to prevent any conflicts with jQuery used from website slice up on which profiler is included.

wouterj · 2015-02-10T13:50:01Z

I meant plain polyfills

aik099 · 2015-02-10T14:09:28Z

Some of polyfills, but I don't think that we need one yet:

fabpot · 2015-02-10T16:25:59Z

@aik099 Looks like @stof does not have any more comments :)

aik099 · 2015-02-10T16:34:17Z

All done, but polyfill, which is a bit unclear. I must remind, that I haven't tested this anywhere, so if you can please test in IE and any other browser.

stof · 2015-02-10T16:37:05Z

👍

fabpot · 2015-02-11T10:39:40Z

Thank you @aik099.

…n IE (aik099) This PR was merged into the 2.6 branch. Discussion ---------- [WebProfilerBundle] Fixes event listener attaching error in IE I haven't tested the change, because I don't any working Symfony installation at hand. By the looks of changes it should work. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | not sure (do we have JS tests?) | Fixed tickets | #13447 | License | MIT | Doc PR | Commits ------- 21693e4 [WebProfilerBundle] Fixes event listener attaching error in IE

This PR was merged into the 2.6 branch. Discussion ---------- Fix the toolbar JS for IE | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | n/a - fix the binding of listeners on XMLHttpRequest to always use the addEventListener method. It does not make sense to change it to attachEvent as it is about binding listeners on DOM elements. - fix the feature detection for the event binding on DOM elements: - the attachEvent and addEventListener methods are on DOM elements, not on the document object - the standard method should be preferred in IE versions supporting both methods - avoid JS errors when XMLHttpRequest is not defined by avoiding to override its open method when the object is not there (old IE versions will still not intercept ajax calls though) this is the complete fix for the code submitted in #13636 (the fix in #13684 was incomplete). Commits ------- b7aa171 Fix the toolbar JS for IE

…boudad) This PR was merged into the 2.6 branch. Discussion ---------- [WebProfilerBundle] fixed undefined buttons variable. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | #13974, #13636 | Tests pass? | yes | License | MIT Commits ------- a8c4da1 [WebProfilerBundle] fixed undefined buttons.

This PR was merged into the 2.6 branch. Discussion ---------- Fix the toolbar JS for IE | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | n/a - fix the binding of listeners on XMLHttpRequest to always use the addEventListener method. It does not make sense to change it to attachEvent as it is about binding listeners on DOM elements. - fix the feature detection for the event binding on DOM elements: - the attachEvent and addEventListener methods are on DOM elements, not on the document object - the standard method should be preferred in IE versions supporting both methods - avoid JS errors when XMLHttpRequest is not defined by avoiding to override its open method when the object is not there (old IE versions will still not intercept ajax calls though) this is the complete fix for the code submitted in symfony#13636 (the fix in symfony#13684 was incomplete). Commits ------- b7aa171 Fix the toolbar JS for IE

aik099 mentioned this pull request Feb 10, 2015

symfony 2 profiler bar throws javascript error in IE8 #13447

Closed

pyrech reviewed Feb 10, 2015
View reviewed changes

aik099 force-pushed the 13447-symfony-2-profiler-bar-throws-javascript-error-in-ie8 branch from 7afae0d to b033350 Compare February 10, 2015 10:48

stof reviewed Feb 10, 2015
View reviewed changes

aik099 force-pushed the 13447-symfony-2-profiler-bar-throws-javascript-error-in-ie8 branch from ecc30cc to fef1eaf Compare February 10, 2015 16:31

[WebProfilerBundle] Fixes event listener attaching error in IE

21693e4

aik099 force-pushed the 13447-symfony-2-profiler-bar-throws-javascript-error-in-ie8 branch from fef1eaf to 21693e4 Compare February 10, 2015 16:33

fabpot mentioned this pull request Feb 10, 2015

Fix profiler throwing javascript errors on IE8 & IE9 #13447 #13634

Closed

fabpot merged commit 21693e4 into symfony:2.6 Feb 11, 2015

aik099 deleted the 13447-symfony-2-profiler-bar-throws-javascript-error-in-ie8 branch February 11, 2015 10:42

stof mentioned this pull request Feb 18, 2015

Fix the toolbar JS for IE #13729

Merged

maximecolin mentioned this pull request Mar 2, 2015

[WebProfiler] Form panel js in the profiler is broken #13824

Closed

aitboudad mentioned this pull request Mar 19, 2015

[WebProfilerBundle] fixed undefined buttons variable. #13987

Merged

xelaris mentioned this pull request Sep 26, 2015

[2.8][WebProfilerBundle] Fix search button click listener #15928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebProfilerBundle] Fixes event listener attaching error in IE #13636

[WebProfilerBundle] Fixes event listener attaching error in IE #13636

aik099 commented Feb 10, 2015

pyrech Feb 10, 2015

wvdweij Feb 10, 2015

stof Feb 10, 2015

aik099 Feb 10, 2015

stof Feb 10, 2015

wvdweij Feb 10, 2015

aik099 Feb 10, 2015

aik099 Feb 10, 2015

stof Feb 10, 2015

aik099 Feb 10, 2015

wouterj commented Feb 10, 2015

aik099 commented Feb 10, 2015

wouterj commented Feb 10, 2015

aik099 commented Feb 10, 2015

fabpot commented Feb 10, 2015

aik099 commented Feb 10, 2015

stof commented Feb 10, 2015

fabpot commented Feb 11, 2015

[WebProfilerBundle] Fixes event listener attaching error in IE #13636

[WebProfilerBundle] Fixes event listener attaching error in IE #13636

Conversation

aik099 commented Feb 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Feb 10, 2015

aik099 commented Feb 10, 2015

wouterj commented Feb 10, 2015

aik099 commented Feb 10, 2015

fabpot commented Feb 10, 2015

aik099 commented Feb 10, 2015

stof commented Feb 10, 2015

fabpot commented Feb 11, 2015