angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
96.22k stars 25.49k forks source link

@angular/animations needs Element.prototype.matches polyfill, not documented. #24769

Closed chumbalum closed 2 years ago

chumbalum commented 6 years ago

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:

Current behavior

When using the dependency @angular/animations in my project, the Application stops working in IE11. There are no specific animations implemented so it must be somewhat internal. The following error occurs on every keyevent in an input field: image

Expected behavior

In IE11, the element which doesn't support "matches" is a HTMLDivElement. I tried debugging the behaviour in Chrome but there, the method shown code is not executed.

Minimal reproduction of the problem with instructions

Problem exist in component @angular/animations (BrowserAnimationsModule) . The Problem will be triggered by usage of package ngx-toastr which somewhat uses the animation component. Unfortunately, I can't provide any minimalistic example by now, because stackblitz is not working in IE11. I made an example nevertheless: https://stackblitz.com/edit/angular-3rczyb

What is the motivation / use case for changing the behavior?

Issue stops whole application from working

Environment


package.json:
  "devDependencies": {
    "@angular/cli": "^6.0.8",
    "@angular/compiler-cli": "^6.0.7",
    "@angular/language-service": "^6.0.7",
    "@types/lodash": "^4.14.110",
    "@types/node": "^10.5.1",
    "angular2-template-loader": "^0.6.2",
    "copy-webpack-plugin": "^4.5.2",
    "css-loader": "^0.28.11",
    "file-loader": "^1.1.11",
    "html-loader": "^0.5.5",
    "html-webpack-plugin": "^3.2.0",
    "node-sass": "^4.9.0",
    "path": "^0.12.7",
    "raw-loader": "^0.5.1",
    "sass-loader": "^7.0.3",
    "style-loader": "^0.21.0",
    "to-string-loader": "^1.1.5",
    "ts-loader": "^4.4.2",
    "tslint": "^5.10.0",
    "typescript": "^2.8.3",
    "webpack": "^4.15.0",
    "webpack-cli": "^3.0.8",
    "webpack-dev-server": "^3.1.4",
    "webpack-merge": "^4.1.3"
  },
  "dependencies": {
    "@angular/animations": "^6.0.7",
    "@angular/common": "^6.0.7",
    "@angular/compiler": "^6.0.7",
    "@angular/core": "^6.0.7",
    "@angular/forms": "^6.0.7",
    "@angular/http": "^6.0.7",
    "@angular/platform-browser": "^6.0.7",
    "@angular/platform-browser-dynamic": "^6.0.7",
    "@angular/router": "^6.0.7",
    "@ng-bootstrap/ng-bootstrap": "^2.2.0",
    "@ngx-translate/core": "^10.0.2",
    "@ngx-translate/http-loader": "^3.0.1",
    "angular2-uuid": "^1.1.1",
    "bootstrap": "^4.1.1",
    "core-js": "^3.0.0-beta.3",
    "lodash": "^4.17.10",
    "ngx-toastr": "^8.8.0",
    "rxjs": "^6.2.1",
    "zone.js": "^0.8.26"
  }

Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [x] IE version 11
- [ ] Edge version XX
trotyl commented 6 years ago

https://angular.io/guide/browser-support#optional-browser-features-to-polyfill

chumbalum commented 6 years ago

@trotyl Thanks for the hint. I did not solve my problem yet, but since this seems to be a known behaviour I'll close this issue for now. Thanks

chumbalum commented 6 years ago

An answer of Derek Brown on stack overflow ( https://stackoverflow.com/questions/46715190/error-in-ie-11-browser-exception-object-doesnt-support-property-or-method-m ) pointed to a solution for my specific problem ( https://developer.mozilla.org/en-US/docs/Web/API/Element/matches#Polyfill ). So matches needs to be polyfilled.

polyfill.ts


import 'core-js';
import 'zone.js';

if (!Element.prototype.matches) {
    Element.prototype.matches = Element.prototype.msMatchesSelector;
}

Now it works without any further animation polyfill

Maybe this should somehow be adressed in the documentation? I reopen this ticket, but feel free to close it, if you think this shouldn't be addressed any further.

robwormald commented 6 years ago

@chumbalum thanks for tracking this down. Assigning this to the docs team to get it added to the docs

jenniferfell commented 6 years ago

@chembu @matsko : Please take a look. Probably something we should make sure is mentioned in the animations guide update? Also the browser support page section on polyfills? (perhaps one of those being the primary explanation and the other linking to it) Are there other recommended locations: API reference location?

gkalpak commented 6 years ago

FWIW, ngUpgrade also relies on matches. Using the browser-specific fallbacks seems to work on all supported browsers: https://github.com/angular/angular/blob/82004c76ac46c5c03a4462ebddd739be8f56f73c/packages/upgrade/src/common/downgrade_component_adapter.ts#L290-L299

Maybe animations could use the same.

bitwhipr commented 6 years ago

@gkalpak @chembu @matsko : It appears something similar has already been implemented.

https://github.com/angular/angular/blob/82004c76ac46c5c03a4462ebddd739be8f56f73c/packages/animations/browser/src/render/shared.ts#L152-L165

I believe the following line should be using the && (AND) operator instead of an OR and all will be well.

https://github.com/angular/angular/blob/82004c76ac46c5c03a4462ebddd739be8f56f73c/packages/animations/browser/src/render/shared.ts#L156

gkalpak commented 6 years ago

Oh, I didn't know the helper is already defined. In any care, the conditions seem fine. Looking at the original issue comment, I can't understand how this error can be thrown on IE11, since _inNode || Element.prototype.matches evalutes to false, which means that it should not go into that code-path.

Does anyone have an actual reproduction (that I can run on IE to see the error)?

bitwhipr commented 6 years ago

@gkalpak : I'll work on a reproduction of the issue. _isNode evaluates to true for me on IE11 causing the issue as described. I'm using webpack and testing the application using webpack-dev-server. I notice @chumbalum is using webpack as well. Perhaps somewhere in there the process variable is getting set.

Also, the only reason I sugested changing the condition was the thought that even if _isNode is true, it might make sense to ensure Element.prototype.matches is defined before it's used in _matches.

Perhaps a better suggestion would be to change the condition to the following:

((_isNode && Element.prototype.matches) || Element.prototype.matches)

gkalpak commented 6 years ago

The condition is fine (because if it is Node, then we know that the implementation (domino) supports matches()). But having _isNode === true outside of Node is definitely wrong (and might also affect other things). We definitely need to find out what's going on and fix it :smiley:

gkalpak commented 6 years ago

Maybe we just need a more robust test for Node (such as this one or something more reliable).

gkalpak commented 6 years ago

Interestingly, the _isNode part seems to be stripped of by the build-optimizer, so it is likely only an issue for code that is not processed by build-optimizer.

For reference, the optimized code looks like this:

if (typeof Element != 'undefined') {
    // this is well supported in all browsers
    _contains = function (elm1, elm2) { return elm1.contains(elm2); };
    if (Element.prototype.matches) {
        _matches = function (element, selector) { return element.matches(selector); };
    }
    else {
        var proto = Element.prototype;
        var fn_1 = proto.matchesSelector || proto.mozMatchesSelector || proto.msMatchesSelector ||
            proto.oMatchesSelector || proto.webkitMatchesSelector;
        if (fn_1) {
            _matches = function (element, selector) { return fn_1.apply(element, [selector]); };
        }
    }
    ...
gkalpak commented 6 years ago

Hm...I get the same for unoptimized code, so no idea what is stripping the _isNode part off. @johnston4hooah, I'll rather wait for your reproduction :grin:

bitwhipr commented 6 years ago

@gkalpak My apologies for my ignorance. I'm relatively new to Angular so thank you for bearing with me 😁. I was able to put together a quick reproduction (https://github.com/johnston4hooah/angular-issue-repro-24769) as a basic app that demonstrates the issue.

You should be able to just do a npm install and then npm start and browse to it on IE11. You can mouse over the Angular logo to produce the error or navigate between router links.

Please let me know if this helpful.

gkalpak commented 6 years ago

Thx for the reproduction, @johnston4hooah. So, I guess you use a custom build setup (which means you will miss on all the great optimizations you get for free with @angular/cli :wink:) and that's why I saw different behavior with a cli-based app.

I see that you define a process.env var, which tricks our check to think it is inside Node.js. For this particular case, we might be able to have a more reliable feature detection (e.g. check if domino is loaded instead of _isNode. But patching environments like this will eventually break something somewhere :smiley:

As a workaround, manually polyfilling Element.prototype.matches is fine imo.

gkalpak commented 6 years ago

Wow, apparently webpack brings in a process polyfill (along with other Node stuff) if you use them. So, merely having the typeof process !== 'undefined' check, makes webpack polyfill process by default. (This can be turned off in the webpack config with node: {process: false}.)

So, overall, I think:

  1. It would be nice if we could detect domino directly (instead of indirectly via typeof process) here.
  2. If you are using a custom build setup, you need to be aware that webpack might polyfill some API (and disable them when not needed).

cc @vikerman (who knows more about domino)

bitwhipr commented 6 years ago

@gkalpak After looking into the Angular CLI, I see that it has come a long way since I last used it. Thanks to your suggestion I will revisit using it. I believe I should be able to not use the process.env var and remove it from my webpack configuration. I really appreciate you spending the time to track this one down.

All the best,

UberMeatShield commented 6 years ago

Just so maybe a search indexer will find this and save another person time, this is also required in PhantomJS if you are using it for test & CI.

Ended up using this in the polyfill, now I I just need to make the compiler complain less about it. https://developer.mozilla.org/en-US/docs/Web/API/Element/matches

kenjiqq commented 6 years ago

We just ran into this issue as well when building with webpack. IE11 threw so many "Object doesn't support property or method 'matches'" errors that the page locked up and died.

Adding the polyfill to work around it.

thw0rted commented 6 years ago

Is there an issue tracking _isNode === true under webpack, specifically? If not, shouldn't there be? I was also bitten by this.

Roadirsh commented 6 years ago

In completion of the previous answer, if you need support for Android 4.1 Webkit browser and IE <= 11 / Edge <=14 you will also need to add this to your polyfill.ts :

if (!Element.prototype.matches) {
    Element.prototype.matches = Element.prototype.msMatchesSelector || Element.prototype.webkitMatchesSelector;
}
tomek-d commented 6 years ago

I faced the same issue today and looks like the problem is not in agnular but in webpack who replaces process global variable in place. Here's webpack plugin responsible for that: https://github.com/webpack/webpack/blob/v4.20.2/lib/node/NodeSourcePlugin.js Looks like it's enabled by default in webpack 4.x. To disable it add node:false to your webpack config (https://github.com/webpack/webpack/blob/3b6d1495c3858d59cfdb46b2343c07f2e4c1316d/lib/WebpackOptionsApply.js#L107) It helped in my case.

jackmusick commented 6 years ago

@tomek-d How is this accomplished with the Angular CLI? I don't have a webpack config in my project.

thw0rted commented 6 years ago

FWIW, there are times when node: false will break other parts of the build -- I'm building a library that includes conditional, node-specific code, with stuff like

if (!("XMLHttpRequest" in window)) {
  const http=require("http");
  ....
}

that can never be reached in practice, in the target (browser) environment, but which Webpack still has to handle at build-time. The simplest way to do this is with node: {process: false, http: "empty"}, which replaces the require call with an empty object. The rest of the compilation process works fine, and since the code is never actually called we don't inject an expensive polyfill.

tomek-d commented 6 years ago

@jackmusick I'm not using angular-cli so for me it was easy. But I would be inerested to see how to solve that problem with angular-cli.

madhav5589 commented 6 years ago

@gkalpak Thank you for the solution. It worked in my case. After upgrading Angular project from 5 to 6, I started getting this error on IE11.

bryiantan commented 6 years ago

@thw0rted

The simplest way to do this is with node: {process: false, http: "empty"}, which replaces the require call with an empty object. The rest of the compilation process works fine, and since the code is never actually called we don't inject an expensive polyfill.

It works. I don't see any issue yet. Have you notice any issue in your application?

thw0rted commented 6 years ago

So far, so good. I still would prefer if it were possible to somehow conditionally exclude the relevant code / file completely when not run from a Node environment, but I don't think it's possible without introducing another dependency.

marcelnem commented 6 years ago

When trying ng test --browsers IE my unit tests for a component which have animations fail with error:

TypeError: Object doesn't support property or method 'matches'

I was unable to fix it using

import 'core-js';
import 'zone.js';

if (!Element.prototype.matches) {
    Element.prototype.matches = Element.prototype.msMatchesSelector;
}

when I try to add this polyfill snippet, angular cli it fails for both ng serve or ng test saying:

ERROR in src/polyfills.ts(53,49): error TS2339: Property 'msMatchesSelector' does not exist on type 'Element'.

thw0rted commented 6 years ago

In an ambient (global) typings file, you can add

    interface Element {
        msMatchesSelector(selectors: string): boolean;
    }

You could also write the polyfill as Element.prototype.matches = (<any>Element.prototype).msMatchesSelector;. Arguably, lib.dom.d.ts could include all vendor-specific methods like this, but you wouldn't want people to go using them by accident.

th0r commented 5 years ago

Element.prototype.matches = (<any>Element.prototype).msMatchesSelector is not a proper solution. It doesn't fix the root of the problem. The proper workaround is adding node: {process: false} to the Webpack's configuration (docs).

BorntraegerMarc commented 5 years ago

@th0r

The proper workaround is adding node: {process: false} to the Webpack's configuration

Unfortunately this did not work for me:

"build":{  
   "builder":"@angular-builders/custom-webpack:browser",
   "options":{  
      "customWebpackConfig":{  
         "path":"./extra-webpack.config.js",
         "mergeStrategies":{  
            "node":"replace"
         }
      }
   }
}

with the extra-webpack.config.js:

module.exports = {
    node: {
        process: false
    }
};
jbogarthyde commented 5 years ago

George thinks that the proper fix would be to detect Node more reliably (as mentioned above). Removing comp: docs label.

mikejpeters commented 5 years ago

When using Apollo GraphQL with Angular, it's dependency ts-invariant stubs process, which also causes this issue.

In situations like this (where a 3rd party library stubs process) modifying the webpack config to set process = false won't resolve the issue; until a proper fix is made, I think the best solution is to add the polyfill yourself in polyfills.ts as suggested above.

markudevelop commented 5 years ago
if (!Element.prototype.matches) {
  Element.prototype.matches = 
  (<any>Element.prototype).matchesSelector || 
  (<any>Element.prototype).mozMatchesSelector ||
  (<any>Element.prototype).msMatchesSelector || 
  (<any>Element.prototype).oMatchesSelector || 
  (<any>Element.prototype).webkitMatchesSelector ||
      function(s) {
        var matches = (this.document || this.ownerDocument).querySelectorAll(s),
            i = matches.length;
        while (--i >= 0 && matches.item(i) !== this) {}
        return i > -1;            
      };
}

Official from mozilla developer with typescript fixed.

th0r commented 5 years ago

I ended up with this workaround in webpack config because {node: {process: false}} was conflicting with some vendor dependencies that were relying on presence of the global process object:

{
  test: /@angular[\\/]animations[\\/].+?[\\/]browser\.js$/,
  loader: "string-replace-loader",
  options: {
    // Replace all the calls to `isNode()` with `false` except `function isNode()`
    search: "(?<!function\\s+)isNode\\(\\)",
    replace: "false",
    // Needed for regexp replacement (https://github.com/Va1/string-replace-loader#regex-replacement)
    flags: ""
  }
}
swseverance commented 5 years ago

Maybe we just need a more robust test for Node (such as this one or something more reliable).

@gkalpak Given that the code already has an isBrowser is it reasonable to update isNode to the following?

export function isBrowser() {
  return (typeof window !== 'undefined' && typeof window.document !== 'undefined');
}

export function isNode() {
  return !isBrowser();
}
gkalpak commented 5 years ago

I don't know much about this part of the code, but your suggestions seems reasonable.

Annie-Huang commented 4 years ago

Does any one know if it is this fixed for angular 9? Or I still need to keep this Element.prototype.matches code block in?

bryiantan commented 4 years ago

have you try the work around mentioned by @thw0rted? I'm using v8.x.x, no issue so far. I had that issue when using v5.x.x several years ago.

jessicajaniuk commented 2 years ago

Thanks for reporting this issue. This issue is now obsolete due to changes in the recent releases. Please update to the most recent Angular version.

If the problem still exists in your application, please open a new issue and follow the instructions in the issue template.

angular-automatic-lock-bot[bot] commented 2 years ago

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.