Skip to content

Commit

Permalink
[WebProfilerBundle] Fixes event listener attaching error in IE
Browse files Browse the repository at this point in the history
  • Loading branch information
Alexander Obuhovich committed Feb 10, 2015
1 parent 8b10043 commit 21693e4
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 19 deletions.
Expand Up @@ -281,14 +281,7 @@
}
// attach listener for expanding/collapsing the target
buttons[i].addEventListener('click', function (e) {
toggle(this);
e.preventDefault();
e.stopPropagation();
return false;
});
clickHandler(buttons[i], toggle);
if (states.hasOwnProperty(targetId)) {
// open or collapse based on stored data
Expand Down Expand Up @@ -380,14 +373,7 @@
throw "Tab target " + targetId + " does not exist";
}
tabs[i].addEventListener('click', function (e) {
select(this);
e.preventDefault();
e.stopPropagation();
return false;
});
clickHandler(buttons[i], select);

This comment has been minimized.

Copy link
@maximecolin

maximecolin Mar 2, 2015

Contributor

Should not it be clickHandler(tabs[i], select); ? Anyway this code breaks completely the form panel in the profiler.

This comment has been minimized.

Copy link
@stof

stof Mar 2, 2015

Member

it should, indeed

This comment has been minimized.

Copy link
@aik099

aik099 Mar 2, 2015

@maximecolin, please create the PR for that. Maybe also for other stuff in comments below. I see many people reporting issues in this commit, but no related PRs are created.

This comment has been minimized.

Copy link
@stof

stof Mar 2, 2015

Member

@aik099 most stuff in the comments below have been fixed by my PR. I missed this typo though.

but in any case, commenting on a commit is the wrong way to report issue. You should either open an issue or a PR. Comments on a commit will get lost (the only way to know them is the notification when they are added), which means that issues are likely to be forgotten if they are not reported as issues

This comment has been minimized.

Copy link
@aik099

aik099 Mar 2, 2015

@aik099 most stuff in the comments below have been fixed by my PR. I missed this typo though.

Wasn't aware of that. Maybe you can reference that PR somewhere in here?

but in any case, commenting on a commit is the wrong way to report issue. You should either open an issue or a PR. Comments on a commit will get lost (the only way to know them is the notification when they are added), which means that issues are likely to be forgotten if they are not reported as issues

Completely agree. I've just replied to other's comments, not posting my new ones.

This comment has been minimized.

Copy link
@stof

stof Mar 2, 2015

Member

Well, read the last comment. The link is in it

Sfjs.addClass(target, 'hidden');
}
Expand All @@ -405,7 +391,26 @@
}
var tabTarget = new TabView(),
toggler = new Toggler(new JsonStorage(sessionStorage));
toggler = new Toggler(new JsonStorage(sessionStorage)),
clickHandler = function (element, callback) {
Sfjs.addEventListener(element, 'click', function (e) {
if (!e) {
e = window.event;
}
callback(e.target || e.srcElement);
if (e.preventDefault) {
e.preventDefault();
} else {
e.returnValue = false;
}
e.stopPropagation();
return false;
});
};
tabTarget.initTabs(document.querySelectorAll('.tree .tree-inner'));
toggler.initButtons(document.querySelectorAll('a.toggle-button'));
Expand Down
Expand Up @@ -171,6 +171,18 @@
requestCounter[0].className = className;
};
var addEventListener;
if (document.addEventListener) {

This comment has been minimized.

Copy link
@stof

stof Feb 11, 2015

Member

this should be document.documentElement.addEventListener actually

This comment has been minimized.

Copy link
@aik099

aik099 Feb 11, 2015

Are you sure, that document.documentElement exists in all browser/browser versions?

Then I guess nobody tested that code.

addEventListener = function (element, eventName, callback) {
element.attachEvent('on' + eventName, callback);
};
} else {
addEventListener = function (element, eventName, callback) {
element.addEventListener(eventName, callback, false);
};
}
{% if excluded_ajax_paths is defined %}
var proxied = XMLHttpRequest.prototype.open;
Expand All @@ -189,7 +201,7 @@
requestStack.push(stackElement);
this.addEventListener("readystatechange", function() {
addEventListener(this, 'readystatechange', function() {

This comment has been minimized.

Copy link
@stof

stof Feb 12, 2015

Member

the issue here is that it should not use the addEventListener method, because it is not a DOM listener. this is the XMLHttpRequest. So the change on this line should be reverted

This comment has been minimized.

Copy link
@aik099

aik099 Feb 12, 2015

I wish all that stuff that pops up now, after PR was merged, would have been discovered during initial PR review.

Funny thing is, that this particular place was reported as not working in the original task. So reverting it back would do nothing good anyway.

if (self.readyState == 4) {
stackElement.duration = new Date() - stackElement.start;
stackElement.loading = false;
Expand All @@ -199,7 +211,7 @@
Sfjs.renderAjaxRequests();
}
}, false);
});
Sfjs.renderAjaxRequests();
}
Expand All @@ -219,6 +231,8 @@
setPreference: setPreference,
addEventListener: addEventListener,
request: request,
renderAjaxRequests: renderAjaxRequests,
Expand Down

13 comments on commit 21693e4

@shtumi
Copy link

@shtumi shtumi commented on 21693e4 Feb 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes broke profiler at Chrome and FF for me:
TypeError: element.attachEvent is not a function

@phansys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I have no time to check this, but after pulled this update I'm experiencing the following issue in Google Chrome Version 40.0.2214.111 (64-bit): Uncaught SyntaxError: Unexpected token u

@aik099
Copy link

@aik099 aik099 commented on 21693e4 Feb 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phidah , any stack trace of this error?

And please don't use dev branches of Symfony on production websites.

@phansys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some sort of debug, I found that the error is thrown inside jQuery (v1.11.1) XHR call (onreadystatechange value is always 0), but note in the stack trace that line 73 at document request is where the debug toolbar script is placed in my HTML structure:

stack: "TypeError: undefined is not a function↵    at addEventListener (http://example.com/:73:7224)↵    at XMLHttpRequest.open (http://example.com/:73:8146)↵    at Object.m.ajaxTransport.send (http://example.com/js/98707ec_jquery.min_1.js:4:25247)↵    at Function.m.extend.ajax (http://example.com/js/98707ec_jquery.min_1.js:4:21302)↵    at Object.generalController.openLogin (http://example.com/js/2660862_general-controller_40.js:783:9)↵    at HTMLButtonElement.<anonymous> (http://example.com/js/2660862_general-controller_40.js:768:14)↵    at HTMLButtonElement.m.event.dispatch (http://example.com/js/98707ec_jquery.min_1.js:3:8436)↵    at HTMLButtonElement.m.event.add.r.handle (http://example.com/js/98707ec_jquery.min_1.js:3:5146)"

@aik099
Copy link

@aik099 aik099 commented on 21693e4 Feb 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this has something to do with way how we set readystatechange event handler. Anybody else experiencing this?

@yellow1912
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing for me, same error. The 'element' here is a XMLHttpRequest and does not have the method attachEvent. Happens in Chrome and FF latest version

@azatyan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have some error.
stack: "TypeError: undefined is not a function↵ at addEventListener (

@sgehrig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - does not work on Chrome.

@aik099
Copy link

@aik099 aik099 commented on 21693e4 Feb 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People, stop writing same here. Just submit PR with a fix and make everybody else happier. 😉

@sgehrig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't want to discuss that as I'm fine with that as long as the issue is somewhere on the list. But

Anybody else experiencing this?

seems like a question to me.

@aik099
Copy link

@aik099 aik099 commented on 21693e4 Feb 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess at least 5 people do. If have an Internet Explorer around, where you can test then please try doing what @stof suggested in 21693e4#commitcomment-9697996 and see if that helps.

@maximecolin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm these lines broke the debug tool bar in chrome.

@stof
Copy link
Member

@stof stof commented on 21693e4 Feb 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone having an issue, please try #13729 to confirm that it fixes it for you

Please sign in to comment.