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

write correct value to native input node on visibility toggle #14

Merged
merged 3 commits into from
Nov 25, 2018

Conversation

oktav777
Copy link
Contributor

@oktav777 oktav777 commented Oct 16, 2018

When input visibility is manipulated by angular renderer, for example hide/show using *ngIf, it will write to the native HTMLInputElement.value the FileInput object from the @Input() value instead of the array of files.

The PR will fix the issue by verifying the type of object before assigning the to the native input value.

@merlosy
Copy link
Owner

merlosy commented Oct 29, 2018

I think this one deserves some thinking.
First, I think this is breaking the current behavior.
Second, native type for <input type=file> is of type FileList, whose File objects are accessible through the files property. So it doesn't seem a native thing to read the array of files directly from the model value.

If this is really an issue please provide a repro case on stackblitz or implements a test case that emphasize the wrong behavior.

@oktav777
Copy link
Contributor Author

Take a look at this demo
Here are the steps to reproduce:

  1. Fill the file input.
  2. Hit the toggle button, this will hide it from the DOM.
  3. Hit again toggle, it will error and the file input will 'lose' it's value.

You can perform this action on the text input and you will see that after re-appearing in DOM, text input will preserve it's value. As I see this is the behavior of the ControlValueAccessor.

Copy link
Owner

@merlosy merlosy left a comment

Choose a reason for hiding this comment

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

Test are ok.

@merlosy merlosy merged commit 2bb6819 into merlosy:master Nov 25, 2018
@merlosy
Copy link
Owner

merlosy commented Nov 25, 2018

Tested and it seems to be ok.
Thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants