-
Notifications
You must be signed in to change notification settings - Fork 27.6k
fix($$RAFProvider): prevent a JavaScript error in Firefox #16192
base: master
Are you sure you want to change the base?
Conversation
Firefox raises a Javascript Error "TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window." with animated elements. This is because Window.requestAnimationFrame() is called without binding to a Window instance in the function which is returned from $$RAFProvider().
src/ng/raf.js
Outdated
@@ -13,9 +13,9 @@ function $$RAFProvider() { //rAF | |||
var rafSupported = !!requestAnimationFrame; | |||
var raf = rafSupported | |||
? function(fn) { | |||
var id = requestAnimationFrame(fn); | |||
var id = requestAnimationFrame.bind($window, fn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you mean to use call
instead of bind
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Thanks! I'll commit a fix in a minute.
src/ng/raf.js
Outdated
return function() { | ||
cancelAnimationFrame(id); | ||
cancelAnimationFrame.bind($window, id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you mean to use call
instead of bind
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Thanks! I'll commit a fix in a minute.
Hi Frederik (or other reviewers), the failed test only complains that the commit message (in my forked repository) does not adhere to the required format. Can this can be resolved when the pull request is merged? Apart from this, the code changes seem to pass the tests. Let me know if you require something else from me. |
Commit message can be changed using interactive rebase. Regarding the effecting change this creates, @gkalpak or @Narretz will have to review it. I only wanted to mention the usage of
|
Under which circumstances does this happen? It's reported like it happens always but this seems unlikely. We need a test or at least a reproduction that shows the error. |
I had difficulties isolating the problem as requested. In particular, I still do not understand the difference in Firefox and Chromium behavior. I suggest to let this pull request rest until I know better what is going on on my particular system. Thanks to the reviewers anyway—this is not as easy as I thought initially. |
I am reopening this pull request with a minimal example to reliably reproduce the error that this pull request fixes. I constructed a minimal browser extension in the directory Instructions:
This error vanishes with the changes to |
I am still not sure what exactly the difference between Firefox and Chromium is. Here is my current guess: it seems as if in Firefox, there are two different functions Try it yourself: edit angular.js locally and insert the following function definition:
Then, insert
Now test the extension in both browsers as described above. Firefox reports Does this give anyone insight into the |
I tried to reproduce, but couldn't get the extension to work on Firefox 55. Can you? |
I am using Firefox 55.0.3 (64-bit) (on Windows 10) and I can't get the temp extension to load when I include the angular.js file in "//": "This works"
"content_scripts": [
{
"matches": ["..."],
"js": [
"test.js"
]
}
]
"//": "This doesn't :("
"content_scripts": [
{
"matches": ["..."],
"js": [
"test.js",
"angular.js"
]
}
] |
But I was able to reproduce the problem with // test.js
var raf1 = requestAnimationFrame;
var raf2 = window.requestAnimationFrame;
console.log(raf1 === raf2); // false
raf1(() => console.log(1)); // Works
raf2(() => console.log(2)); // TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window. I also tried with Firefox Nightly and it still doesn't work, although there is no error logged to the console 😕 TBH, this sounds like a bug in Firefox. I wonder what else might be affected (besides |
Hi @gkalpak, (1) the demo extension works for me in Firefox 55.0.3 (32-bit) on Windows 10. However, since you are able to reproduce the problem without angular.js, let's not worry about this. (2) If you don't see the error logged on the (primary) console, try the “Debug” button in Firefox's “about:debugging” page. This opens a second console, on which you will likely see the error message. (3) I don't think that this is a bug in Firefox but I suspect that this is intended behavior. Let me know if you feel I should still report this to the Firefox community. Here are my arguments: The error is due to the question what the global object is in the JavaScript scope when
reports However, if the JavaScript code is executed as part of an extension, the global object is not necessarily the global To me, the sanest solution is to bind the
so we should call |
¯\_(ツ)_/¯
Like I said, I do see the error in Firefox 55. I don't see it in Firefox Nightly.
I still think this is a bug in Firefox. When calling a function previously assign to a variable (when in strict mode as we are here), it should run with Also, the following will work in a normal (non-extension) environment in Firefox: var raf1 = requestAnimationFrame;
var raf2 = window.requestAnimationFrame;
raf1(cb);
raf1.call(undefined, cb);
raf1.call(null, cb);
raf1.call(this, cb);
raf2(cb);
raf2.call(undefined, cb);
raf2.call(null, cb);
raf2.call(window, cb); From the above, only the last one works from an extension. IMO, this is a clear indication that something is wrong. I am fine working aroung this in AngularJS (as you suggested) - it won't be the first time we work around browser-specific bugs - but I would still report it to Firefox and see what they think. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix: prevent a JavaScript error in Firefox.
What is the current behavior? (You can also link to an open issue here)
Firefox raises a Javascript Error "TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window." with animated elements. This is because Window.requestAnimationFrame() is called without binding to a Window instance in the function which is returned from $$RAFProvider().
What is the new behavior (if this is a feature change)?
No error message.
Does this PR introduce a breaking change?
Not that I know of.
Please check if the PR fulfills these requirements
Other information:
I was not able to run local tests.