fix(datepicker): notify cva for any input change

[alexluecke]

fixes #13410

[mmalerba]

We probably want to call _cvaOnChange before emitting. How about doing it like this:

this._value = date;
this._cvaOnChange(date);

if (!this._dateAdapter.sameDate(date, this._value)) {
  this._valueChange.emit(date);
  this.dateInput.emit(new MatDatepickerInputEvent(this, this._elementRef.nativeElement));
}

[alexluecke]

Sounds good

[alexluecke]

Actually, this won't work. You will end up never calling the dateInput output since this._value will always be value date.

[mmalerba]

Oh, yeah you can just move the check before

const shouldEmit = this._dateAdapter.sameDate(date, this._valie);

[crisbeto]

The code looks good, but it definitely needs a unit test, otherwise I can see the issue being reintroduced easily by somebody moving the function call a couple of lines down.

[alexluecke]

@crisbeto I think in order to correctly emit input change events, it needs to keep track of the input value in the component, and emit when that changes. As is, the code will not notify of input changes when the input value changes (i.e. the user types in a value 'bad' 'bad2', the date input event is not fired). As such, I think we should probably do something like:

...
  private previousInputValue: string = '';
...

 _onInput(value: string): void {
    let date = this._dateAdapter.parse(value, this._dateFormats.parse.dateInput);
    this._lastValueValid = !date || this._dateAdapter.isValid(date);
    date = this._getValidDateOrNull(date);

    const previousValue = this._value;

    this._value = date;
    this._cvaOnChange(date);

    if (!this._dateAdapter.sameDate(date, previousValue)) {
      this._valueChange.emit(date);
    }

    if (value !== this.previousInputValue) {
      this.previousInputValue = value;
      this.dateInput.emit(new MatDatepickerInputEvent(this, this._elementRef.nativeElement));
    }
  }

The reason we need to maintain a input value model on the class is because this._elementRef.nativeElement.value will always contain the current value of the input.

Thoughts?

[mmalerba]

@alexluecke I do agree with moving the input event outside of that if, it would be consistent with what a native number input does then when you type a string like 'eee' (https://stackblitz.com/edit/angular-material2-issue-xd5q3g?file=app/app.component.ts). Since this is all happening inside the _onInput method, I think you can just always emit an input event and not have to save the previousInputValue.

[alexluecke]

@mmalerba the reason I am trying to avoid that is @crisbeto had a specific PR for avoiding that behavior. I actually agree that the input should always emit anytime an input event occurs irrespective of whether or not the input value is actually different. At which time it just seems like we should revert the previous PR changes.

[alexluecke]

• with the exception of only emiting new dates when they actually change.

[mmalerba]

@crisbeto was there a specific bug you were trying to fix with #10952? It does seem like a native type=number input emits an input event when I mark a character and replace it with the same character. Shouldn't our dateInput event work the same way?

[crisbeto]

I don't remember whether there was a specific bug, but aside the thing I mention in the PR description, there's also a bug on IE where the input event gets fired on init if it has a placeholder. AFAIK if we didn't have the check in place, the input would start off as dirty on IE. I think I had another PR that does more or less the same, but it never ended up getting in because of a presubmit issue.

[alexluecke]

Could I get some steps to reproduce?

[crisbeto]

You can see it by opening this one IE. It'll trigger an alert when the input event fires. Also here's how we've resolved it for MatAutocomplete: https://github.com/angular/material2/blob/master/src/lib/autocomplete/autocomplete-trigger.ts#L405

[alexluecke]

@crisbeto That link in the comment to https://docs.microsoft.com/en-us/collaborate/connect-redirect seems to no longer be viable. I updated the PR to always emit events except when the input is not the active document element.

[crisbeto]

LGTM. cc @mmalerba for final approval.

[ngbot]

I see that you just added the pr: merge ready label, but the following checks are still failing:
     status "continuous-integration/travis-ci/pr" is failing
     missing required labels: target: *

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

[mmalerba]

Marking this as target: patch for now, but may have to change to minor if it breaks too many unit tests

[ngbot]

Hi @alexluecke! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[alexluecke]

@mmalerba added a few commits after needing to rebase a conflict. Any idea when this is going to get merged?

[mmalerba]

We test all PRs against Google's codebase before merging. It looks like this one has several failures that will need to be investigated before it can be merged

[andrewseguin]

Closing since the attached issue appears to be fixed and this has not been rebased for a long time. If you find that this is still an issue, please feel free to re-open it with a new pull request

[angular-automatic-lock-bot]

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