Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(platform-browser): add unique input event plugin #33688

Open
wants to merge 1 commit into
base: master
from

Conversation

@DerSizeS
Copy link

DerSizeS commented Nov 8, 2019

This plugin checks if value of element is changed before firing input event



Internet Explorer 10+ implements 'input' event but it erroneously fires under various situations, e.g. when placeholder changes, when non english placeholder is used or a control is focused

Fixed only from Edge 15.15002
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/101220/
This bug will not be fixed in Internet Explorer
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11405058/

Closes #14440, #17951, #15299, #16151

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Internet Explorer 10+ implements 'input' event but it erroneously fires under various situations, e.g. when placeholder changes, when non english placeholder is used or a control is focused

Issue Number: #14440, #17951, #15299, #16151

What is the new behavior?

If plugin config is provided – input event is fired when value of element is changed.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Here is the problem reproduction (to fix IE behavior- uncomment the provider): https://stackblitz.com/edit/angular-feu5wd?embed=1&file=src/app.module.ts

I had to recreate the PR due to the accidental deletion of the associated fork.

@googlebot googlebot added the cla: yes label Nov 8, 2019
@DerSizeS DerSizeS force-pushed the DerSizeS:fix-ie-input-event branch from 760af48 to 642ff5e Nov 8, 2019
This plugin checks if value of element is changed before firing input event



Internet Explorer 10+ implements 'input' event but it erroneously fires under various situations, e.g. when placeholder changes, when non english placeholder is used or a control is focused
Fixed only from Edge 15.15002 https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/101220/
This bug will not be fixed in Internet Explorer https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11405058/

Closes #14440, #17951, #15299, #16151
@DerSizeS DerSizeS force-pushed the DerSizeS:fix-ie-input-event branch from 642ff5e to ceea182 Nov 8, 2019
@DerSizeS DerSizeS mentioned this pull request Nov 8, 2019
6 of 14 tasks complete
@DerSizeS DerSizeS marked this pull request as ready for review Nov 8, 2019
@DerSizeS DerSizeS requested review from angular/docs-infra as code owners Nov 8, 2019
@kara kara added the comp: core label Nov 8, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 8, 2019
Copy link
Member

petebacondarwin left a comment

This is a really nice piece of work @DerSizeS - thanks for submitting it.

I am torn though.

Since this is only fixing a bug in a small set of browsers (IE10 and IE11, right?) it feels like it is too much code to add to the standard framework. The event manager architecture is already in place in the framework to allow this to be added on a per project basis without having to modify the core.

I feel that a more effective solution, which would not impact projects that do not require IE support, would be to have this as some kind of extra module that a developer could add to their app as they need it - a bit like the polyfills approach we have already. But I do feel like this should be easily available to developers without having to go look for some 3rd party library to do this...

@IgorMinar and @kara do you have a view on this?

@DerSizeS

This comment has been minimized.

Copy link
Author

DerSizeS commented Nov 13, 2019

How about exposing UniqueInputEventPlugin itself? That way developers could use FactoryProvider for conditional enabling plugin.

Example:

export function setupUniqueInputEventManagerPlugin(document: Document) {
  if (isIE()) {
    return new UniqueInputEventPlugin(document);
  }
  return NOOP_EVENT_MANAGER_PLUGIN;
}
providers: [
  {
    provide: EVENT_MANAGER_PLUGINS,
    useFactory: setupUniqueInputEventManagerPlugin,
    deps: [DOCUMENT],
    multi: true,
  },
],
@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 14, 2019

That would still leave the code in framework, which is what I am unsure about. We need a call from @IgorMinar or @kara, I think.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 16, 2019

this is a great PR, but it also increases the payload size for all apps.

We can merge it only If we figure out how to make this provider optional or IE specific.

I don't believe that there is a way in the CLI to include some code only into the legacy es5 bundle or is there, @filipesilva @clydin?

useClass: UniqueInputEventPlugin,
multi: true,
deps: [DOCUMENT, [new Optional(), UNIQUE_INPUT_EVENT_PLUGIN_CONFIG]]
},

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 16, 2019

Member

So if we excluded this block of code from the library (i.e from this PR) then I believe it would get tree shaken by default and not add to the bundle size. But then the onus is on the app developer to find out about this and add it back in for their IE scenarios...

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Nov 17, 2019

@DerSizeS - I think we are agreed that we could land this if we only implement and export the UniqueInputEventPlugin but do not wire it up. We should also add a section to the API docs for it, explaining when it should be used and how it can be provided. By the way, I think it could just use the useClass provider, why did you suggest that the developer would need to use the useFactory approach?

@DerSizeS

This comment has been minimized.

Copy link
Author

DerSizeS commented Nov 17, 2019

By the way, I think it could just use the useClass provider, why did you suggest that the developer would need to use the useFactory approach?

With useClass this plugin will be enabled for all browsers, in the case of useFactory developer will be able to control when plugin will be enabled (e.g. IE 10+).

For this scenario, I think it's worth adding NOOP_EVENT_MANAGER_PLUGIN.

@filipesilva

This comment has been minimized.

Copy link
Member

filipesilva commented Nov 18, 2019

Exporting it but not wiring would not increase all users payload size, but would still increase all payload size for users that make use of it. Making it conditional in user code still includes it in bundles.

I don't think we have a way to add things to the es5-only polyfills though. @clydin is there a way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.