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(input): properly determine input value #2455

Merged
merged 3 commits into from
Jan 4, 2017

Conversation

devversion
Copy link
Member

@devversion devversion commented Dec 29, 2016

Right now the MdInputDirective tries to cache the value of the input.

To do this, the MdInputDirective needs to listen for NgControl value changes and for native (change) events.

This will be problematic when a value is set directly to the input element (using [value] property binding) because Angular is only able to recognize this change in the first ChangeDetection.

<md-input-container>
  <input md-input [value]="myValue" placeholder="Label">
</md-input-container>

The approach of updating the value in the ngAfterViewInit lifecycle hook, will result in a binding change while being in a ChangeDetection, which leads to a ChangeDetection error.

Expression has changed after it was checked. Previous value: 'true'. Current value: 'false'.

The credits for the should update the value when using FormControl.setValue test go to @crisbeto

Fixes #2441. Fixes #2363. Fixes #2477

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2016
@jelbourn jelbourn requested review from mmalerba and removed request for jelbourn December 29, 2016 19:37
@kara
Copy link
Contributor

kara commented Dec 29, 2016

@devversion I think this might break turning off floating placeholders, e.g.:

<md-input-container [floatingPlaceholder]="false">
   ...
</md-input-container>

Now that we're not listening to input events, the placeholder sticks around as the user types.

@devversion
Copy link
Member Author

Great catch. I've chatted with @kara about this and we thought about alternate approaches than listening to the (input) event.

To let Angular notice this change we need the input event, which triggers a new round of change detection and then can update the md-empty class on the placeholder.

Similar to angular/angular#default_value_accessor.ts, which should also trigger a change detection.

Need confirmation that the input event will always trigger a new change detection as in the FormsModule.

@fxck
Copy link
Contributor

fxck commented Jan 2, 2017

can you get this at least into master? everything is kinda broken now

image

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick on the comment, otherwise looks good. @mmalerba will probably want to take a look too.

_onInput() { this.value = this._elementRef.nativeElement.value; }
_onInput() {
// This is a noop function and is used to let Angular recognize the changes
// to the input element while typing. Listening to the `input` event is similar to NgControls.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Can you add some more detail here? I'd also add that if you're using one of the FormsModule or ReactiveFormsModule directives, the input event is already watched and will kick off change detection appropriately. But that if you're not using one of these and you've disabled the floating placeholder, we need to ensure that change detection still runs immediately on input to hide the placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment.

@kara
Copy link
Contributor

kara commented Jan 3, 2017

LGTM, waiting on @mmalerba

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 4, 2017
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jan 4, 2017
@kara
Copy link
Contributor

kara commented Jan 4, 2017

@devversion Sorry, one more rebase

Right now the `MdInputDirective` tries to cache the `value` of the input.

To do this, the `MdInputDirective` needs to listen for `NgControl` value changes and for native `(change)` events.

This will be problematic when a value is set directly to the input element (using `[value]` property binding) because Angular is only able to recognize this change in the first ChangeDetection.

```html
<md-input-container>
  <input md-input [value]="myValue" placeholder="Label">
</md-input-container>
```

The approach of updating the value in the `ngAfterViewInit` lifecycle hook, will result in a binding change while being in a ChangeDetection, which leads to a ChangeDetection error.

```
Expression has changed after it was checked. Previous value: 'true'. Current value: 'false'.
```

Fixes angular#2441. Fixes angular#2363
@devversion
Copy link
Member Author

@kara Done.

@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Jan 4, 2017
@kara kara merged commit 3a11927 into angular:master Jan 4, 2017
@devversion devversion deleted the fix/input-value-binding branch January 4, 2017 22:31
@abdulhaq-e
Copy link

hey @devversion I still get this issue if there is a condition check in ngAfterViewInit/Checked. See this plunkr: http://plnkr.co/edit/eyN8D8oZuEwDhHLINsZ5?p=preview

@RicardoVaranda
Copy link

@abdulhaq-e I'm still having the same issue as you using master, if I don't use it in ngAfterViewInit it seems to crash the form and not display anything.

@abdulhaq-e
Copy link

@RicardoVaranda See this comment: #2837 (comment)

You need to use the ngOnChanges life cycle hook.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
7 participants