fix(forms): number input fires valueChanges twice. #36087

TidianeRolland · 2020-03-16T15:32:17Z

Prior to this commit, input field of type number used to fire valueChanges twice. First time after typing in the input field and second time when the input field loses the focus.

PR Close #12540

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

Input field of type number emits valueChanges twice. First time after typing in the input field and second time when the input field loses the focus.

Issue Number: #12540

What is the new behavior?

Input field of type number emits valueChanges once.

Does this PR introduce a breaking change?

Yes
No

Other information

petebacondarwin

LGTM

the linting failure is because you need to format the test file correctly. I setup my VS Code to autorun the clang formatter on every save, which sorts this out
also it would be good if we were able to explain, in the commit message, why the (change) event can now be removed; it was obviously included originally for a reason...

petebacondarwin · 2020-03-16T21:01:32Z

I think this is the reason... #7557 (comment)

petebacondarwin · 2020-03-16T21:03:16Z

According to this we still support IE9 - https://angular.io/guide/browser-support
But I think that in this case there should be a polyfill to cover the problem of input not being triggered...

AndrewKushnir · 2020-03-20T02:07:38Z

Hi @TidianeRolland, thanks for the PR!

Could you please provide a bit more information on this change to help me understand the problem better? Currently we fire 2 events: one after typing in the input field (as a result of the oninput event handler) and another one when the input field loses the focus (due to onchange event handler). Having both events fire at the same time is undesirable, but if I understand correctly, removing one of them might be a breaking change (losing focus will no longer trigger value change event, which might be used by developers)? I'm not proposing to keep the change host listener, just want to understand the impact of this change better.

I also started a g3 presubmit for blueprint and a global one (internal only links) to see if this change affects any targets in g3.

Thank you.

TidianeRolland · 2020-03-20T09:24:32Z

Hi @AndrewKushnir thanks for the reply!

Yes, currently we fire 2 events: one after typing in the input field (as a result of the oninput event handler) and another one when the input field loses the focus (due to onchange event handler). Having both events fire at the same time is undesirable and I think having the onchange event trigger just when the input field loses the focus is also undesirable, according to this issue #12540.
Because a lot of developers are subscribing to the valueChanges observable. valueChanges observable emits twice the same value of the input field.

I noticed that we're triggering the change event only for input type number.
The other types of fields aren't concerned.
I think the input event is enough for input type number too.
I hope I explained well.

Thanks for your help

petebacondarwin · 2020-03-20T09:31:59Z

My understanding, although it may be lost in the annals of time - perhaps as far back as AngularJS, is that the extra handler was only added to support IE9, which appeared not to trigger the input event on some occasions. But @TidianeRolland was not able to recreate that issue with IE9 now, so it may have been fixed in the browser?

AndrewKushnir

Hi @TidianeRolland and @petebacondarwin,

Running global TAP indicated that there are few tests in g3 that rely on the change event to be present, here is an example code (simplified) from test harness:

setTextValue(element, text) {
    element.value = text;
    const changeEvent = new Event('change');
    element.dispatchEvent(changeEvent);
}

as a result, form control doesn't pick up an updated value. This is similar to the test that you've added as a part of this PR.

It can probably be refactored to trigger input event instead, but that would require additional time and investigation. I'm marking this PR with the "breaking change" flag, since there might be other cases outside g3 with the same behavior/expectations.

@TidianeRolland I also looked at the original issue report and I think it'd be great to add one more test using Forms API that would replicate original problem (and make sure that only one event is fired), so I set the status of my review to "requested changes".

Thank you.

atscott · 2020-04-03T16:23:38Z

Thanks to @petebacondarwin for the tip on needing to test IE9 and @gkalpak for helping to test the demo.

It appears that the input event does not fire on IE9 when text in the input is removed via backspace or a cut action. Because IE9 is still a supported browser, I don't think we can merge this change. It would certainly be useful to add a comment about this in the code, noting the change event is needed for IE9 and can be removed when we drop support for it.

atscott · 2020-04-03T16:45:10Z

What's the contract on the valueChanges event of the forms API? Are we able to change it to pipe through a distinctUntilChanged?

AndrewKushnir · 2020-04-03T16:54:20Z

@atscott thanks for additional investigation! 👍

Just want to check if this problem only happens in IE9 or also present in IE10/11?

Thank you.

atscott · 2020-04-03T16:59:00Z

Just want to check if this problem only happens in IE9 or also present in IE10/11?

That's right - I was able to reproduce this missing input event for backspace in a protractor test. The only failure is in IE9, while it passes in chrome, firefox, IE10, and IE11.

TidianeRolland · 2020-04-03T17:09:54Z

Thanks to @petebacondarwin for the tip on needing to test IE9 and @gkalpak for helping to test the demo.

It appears that the input event does not fire on IE9 when text in the input is removed via backspace or a cut action.

@atscott it's really strange because I wasn't able to reproduce this bug using IE9 emulated in IE11 and IE9 on a Windows 7 machine. The input event was triggered

gkalpak · 2020-04-03T17:16:02Z

@TidianeRolland, fwiw I was able to reproduce it using IE9 emulated in IE11 on Windows 10.
Note that the issue only appears when you delete characters from the value (e.g. hitting backspace or cutting). The input event works fine when typing more characters into the field.

atscott · 2020-04-03T17:38:27Z

@TidianeRolland can you confirm that backspace/cut does fire the input event for you in IE9? I find it curious that it's working for you but not for me or George. You can use the demo linked above to test this.

TidianeRolland · 2020-04-03T18:01:01Z

@TidianeRolland can you confirm that backspace/cut does fire the input event for you in IE9? I find it curious that it's working for you but not for me or George. You can use the demo linked above to test this.

I redid my tests. Backspace does not fire input event for emulated IE9.
If I weren't with a friend when I was doing my tests I would have thought I am crazy, because backspace was perfectly working.
@atscott I confirm, stop listening to change event is not a good idea if we're supporting IE9

atscott · 2020-04-03T18:11:21Z

I redid my tests. Backspace does not fire input event for emulated IE9.

Thanks for retesting that. I'm adding the blocked label on this PR for now. @AndrewKushnir is adding an action item to discuss with the team what the plan is for IE9 support in the future. We'll get back to you with what can be done with this after the discussion.

atscott · 2020-05-01T17:58:03Z

Hi @TidianeRolland - We discussed this issue today and decided that we can make this change and add a note in the breaking change on what devs can do if they want to maintain IE9 support.

Please update your commit message to include in your footer:

BREAKING CHANGE: <description> (tests that could break because they rely on 'change' event, requirement for devs to add their own `(change)` listener if they want to support IE9 in the same way it is now, etc)

TidianeRolland · 2020-05-04T16:19:55Z

Hi @TidianeRolland - We discussed this issue today and decided that we can make this change and add a note in the breaking change on what devs can do if they want to maintain IE9 support.

Please update your commit message to include in your footer:
BREAKING CHANGE: <description> (tests that could break because they rely on 'change' event, requirement for devs to add their own `(change)` listener if they want to support IE9 in the same way it is now, etc)

Hi @atscott - It's done

atscott · 2020-05-04T19:09:55Z

One of the internal tests failures took me down a deep rabbit hole. It appears that this change may also break some selenium-webdriver tests if you have an old version.

WebElement.clear() (code link) and WebElement.sendKeys (code link) methods call htmlInput.setValueAttribute(""); which, until 2017 (commit reference), would trigger the 'change' event on the input. From what I can tell right now, this means sendKeys and clear would trigger the onChange handler of the number input but no longer do.

Still investigating what can be done to resolve this issue.

Prior to this commit, number input fields would to fire valueChanges twice: once for `input` events when typing and second for the `change` event when the field lost focus (both events happen at once when using the increment and decrement buttons on the number field). Fixes angular#12540 BREAKING CHANGE: Number inputs no longer listen to the `change` event. * Tests which trigger `change` events need to be updated to trigger `input` events instead. * The `change` event was in place to support IE9, as we found that `input` events were not fired with backspace or cut actions. If you need to maintain IE9 support, you will need to add a change event listener to number inputs and call the `onChange` method of `NumberValueAccessor` manually. * Lastly, old versions of WebDriver would synthetically trigger the `change` event on `WebElement.clear` and `WebElement.sendKeys`. If you are using an old version of WebDriver, you may need to update tests to ensure `input` events are triggered. For example, you could use `element.sendKeys(Keys.chord(Keys.CONTROL, "a"), Keys.BACK_SPACE);` in place of `element.clear()`. PR Close angular#12540

atscott · 2020-05-06T16:10:15Z

global presubmit from a few days ago with a rerun of the failing started today. These should all be green now. New TGP scheduled for the 12:00 train.

atscott · 2020-05-07T01:08:43Z

green TGP

angular-automatic-lock-bot · 2020-06-07T16:10:05Z

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.}

Prior to this commit, number input fields would to fire valueChanges twice: once for `input` events when typing and second for the `change` event when the field lost focus (both events happen at once when using the increment and decrement buttons on the number field). Fixes angular#12540 BREAKING CHANGE: Number inputs no longer listen to the `change` event. * Tests which trigger `change` events need to be updated to trigger `input` events instead. * The `change` event was in place to support IE9, as we found that `input` events were not fired with backspace or cut actions. If you need to maintain IE9 support, you will need to add a change event listener to number inputs and call the `onChange` method of `NumberValueAccessor` manually. * Lastly, old versions of WebDriver would synthetically trigger the `change` event on `WebElement.clear` and `WebElement.sendKeys`. If you are using an old version of WebDriver, you may need to update tests to ensure `input` events are triggered. For example, you could use `element.sendKeys(Keys.chord(Keys.CONTROL, "a"), Keys.BACK_SPACE);` in place of `element.clear()`. PR Close angular#12540 PR Close angular#36087

pullapprove bot requested a review from AndrewKushnir March 16, 2020 15:32

googlebot added the cla: yes label Mar 16, 2020

petebacondarwin approved these changes Mar 16, 2020

View reviewed changes

TidianeRolland force-pushed the angular-issue-12540 branch 2 times, most recently from 2da3d40 to 0b4908b Compare March 17, 2020 12:54

AndrewKushnir added the area: forms label Mar 18, 2020

ngbot bot added this to the needsTriage milestone Mar 18, 2020

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Mar 19, 2020

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Mar 20, 2020

AndrewKushnir added the breaking changes label Mar 20, 2020

AndrewKushnir requested changes Mar 20, 2020

View reviewed changes

atscott added the state: blocked label Apr 3, 2020

AndrewKushnir mentioned this pull request Apr 22, 2020

fix(forms): removed 'change' host binding from NumberValueAccessor #32704

Closed

14 tasks

atscott removed the state: blocked label May 1, 2020

atscott modified the milestones: needsTriage, v10-candidates May 1, 2020

TidianeRolland force-pushed the angular-issue-12540 branch 4 times, most recently from fd0de03 to 2578b7a Compare May 4, 2020 15:41

atscott added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels May 4, 2020

alxhub force-pushed the angular-issue-12540 branch from 2578b7a to 1860e5f Compare May 5, 2020 22:26

alxhub force-pushed the angular-issue-12540 branch from 1860e5f to f5cacbb Compare May 5, 2020 22:26

AndrewKushnir approved these changes May 6, 2020

View reviewed changes

atscott removed the action: review The PR is still awaiting reviews from at least one requested reviewer label May 6, 2020

atscott added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels May 7, 2020

alxhub closed this in 97d6d90 May 7, 2020

IgorMinar mentioned this pull request May 16, 2020

fix(dev-infra): misc fixes for the compare master and patch script #37150

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(forms): number input fires valueChanges twice. #36087

fix(forms): number input fires valueChanges twice. #36087

TidianeRolland commented Mar 16, 2020

petebacondarwin left a comment

petebacondarwin commented Mar 16, 2020

petebacondarwin commented Mar 16, 2020

AndrewKushnir commented Mar 20, 2020 •

edited

TidianeRolland commented Mar 20, 2020

petebacondarwin commented Mar 20, 2020

AndrewKushnir left a comment •

edited

atscott commented Apr 3, 2020

atscott commented Apr 3, 2020

AndrewKushnir commented Apr 3, 2020

atscott commented Apr 3, 2020 •

edited

TidianeRolland commented Apr 3, 2020

gkalpak commented Apr 3, 2020

atscott commented Apr 3, 2020

TidianeRolland commented Apr 3, 2020 •

edited

atscott commented Apr 3, 2020

atscott commented May 1, 2020

TidianeRolland commented May 4, 2020

atscott commented May 4, 2020

atscott commented May 6, 2020

atscott commented May 7, 2020

angular-automatic-lock-bot bot commented Jun 7, 2020

fix(forms): number input fires valueChanges twice. #36087

fix(forms): number input fires valueChanges twice. #36087

Conversation

TidianeRolland commented Mar 16, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin commented Mar 16, 2020

petebacondarwin commented Mar 16, 2020

AndrewKushnir commented Mar 20, 2020 • edited

TidianeRolland commented Mar 20, 2020

petebacondarwin commented Mar 20, 2020

AndrewKushnir left a comment • edited

Choose a reason for hiding this comment

atscott commented Apr 3, 2020

atscott commented Apr 3, 2020

AndrewKushnir commented Apr 3, 2020

atscott commented Apr 3, 2020 • edited

TidianeRolland commented Apr 3, 2020

gkalpak commented Apr 3, 2020

atscott commented Apr 3, 2020

TidianeRolland commented Apr 3, 2020 • edited

atscott commented Apr 3, 2020

atscott commented May 1, 2020

TidianeRolland commented May 4, 2020

atscott commented May 4, 2020

atscott commented May 6, 2020

atscott commented May 7, 2020

angular-automatic-lock-bot bot commented Jun 7, 2020

AndrewKushnir commented Mar 20, 2020 •

edited

AndrewKushnir left a comment •

edited

atscott commented Apr 3, 2020 •

edited

TidianeRolland commented Apr 3, 2020 •

edited