fix(platform): detect and ignore scrollBehavior polyfills #20155
Conversation
crisbeto
added
P3
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
17847/polyfilled-scroll-behavior
branch
from
3e41f09
to
f5e736e
Compare
| // functions are obfuscated using `[native code]`, whereas if it was overwritten we'd get | ||
| // the actual function source. Via https://davidwalsh.name/detect-native-function. Consider | ||
| // polyfilled functions as supporting scroll behavior. | ||
| scrollBehaviorSupported = /\{\s*\[native code\]\s*\}/.test(scrollToFunction.toString()); |
There was a problem hiding this comment.
This is a little hacky, but it seems to work. Another approach I was considering was similar to the getRtlScrollAxisType which creates an element and tries to scroll it which is definitely better, but I decided not to do it, because it doesn't actually detect if scrollBehavior is supported, but rather that the ScrollOption object parameter is supported. While that would fix the original issue of polyfills not being picked up, it would mean that the name of supportsScrollBehavior wouldn't match what it was actually doing.
| // We can detect if the function has been polyfilled by calling `toString` on it. Native | ||
| // functions are obfuscated using `[native code]`, whereas if it was overwritten we'd get | ||
| // the actual function source. Via https://davidwalsh.name/detect-native-function. Consider | ||
| // polyfilled functions as supporting scroll behavior. |
There was a problem hiding this comment.
I don't quite understand- if we treat a polyfill as fully supported, why check for a native implementation?
There was a problem hiding this comment.
The problem is that pretty much all browsers support these two signatures: scrollTo(options: {top: number, left: number, behavior: string}) or scrollTo(top: number, left: number), whereas Safari only supports the latter. If we were to pass in an object to Safari nothing would happen so we have to detect if it's polyfilled. A bit above we detect whether the behavior parameter is supported which is a good indicator that the object is supported too.
There was a problem hiding this comment.
Wouldn't this check return true for Safari, then, when the behavior config isn't supported?
There was a problem hiding this comment.
I messed it up here, because I didn't negate the check. It should be correct now.
There was a problem hiding this comment.
I used to check for focusOptions preventScroll support with this technique:
let supported = false;
document.createElement('div').focus({
get preventScroll() {
supported = true;
return false;
},
});
Looks like it could work here too, to check if browser tries to retrieve value of behavior key in a passed object.
We use the `supportsScrollBehavior` function to check whether to try and pass in the scroll options object to `Scrollable.scrollTo` since passing it in on a browser that doesn't support it won't do anything. The problem is that the only way to detect if scroll behavior is supported is to look for the `scrollBehavior` property on a DOM element's CSS styles, however this will cause scroll behavior polyfills to be ignored since they only modify `Element.prototype.scrollTo`. These changes try to detect whether `scrollTo` has been polyfilled, and if it has, we consider it as being supported. Fixes angular#17847.
crisbeto
force-pushed
the
17847/polyfilled-scroll-behavior
branch
from
f5e736e
to
a7c4ae7
Compare
| // We can detect if the function has been polyfilled by calling `toString` on it. Native | ||
| // functions are obfuscated using `[native code]`, whereas if it was overwritten we'd get | ||
| // the actual function source. Via https://davidwalsh.name/detect-native-function. Consider | ||
| // polyfilled functions as supporting scroll behavior. |
We use the `supportsScrollBehavior` function to check whether to try and pass in the scroll options object to `Scrollable.scrollTo` since passing it in on a browser that doesn't support it won't do anything. The problem is that the only way to detect if scroll behavior is supported is to look for the `scrollBehavior` property on a DOM element's CSS styles, however this will cause scroll behavior polyfills to be ignored since they only modify `Element.prototype.scrollTo`. These changes try to detect whether `scrollTo` has been polyfilled, and if it has, we consider it as being supported. Fixes #17847. (cherry picked from commit 6569041)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
We use the
supportsScrollBehaviorfunction to check whether to try and pass in the scroll options object toScrollable.scrollTosince passing it in on a browser that doesn't support it won't do anything. The problem is that the only way to detect if scroll behavior is supported is to look for thescrollBehaviorproperty on a DOM element's CSS styles, however this will cause scroll behavior polyfills to be ignored since they only modifyElement.prototype.scrollTo.These changes try to detect whether
scrollTohas been polyfilled, and if it has, we consider it as being supported.Fixes #17847.