Enhance FileInput to Update event.target.files on Remove #6136

amandadupell · 2022-05-18T20:08:17Z

What does this PR do?

Allows for a user to access event.target.files in the onChange function when removing a file. Previously, this was undefined because we were passing in the event from the remove button's onClick, not the input's onChange (onClick does not have a files value, only the FileInput's onChange does). This PR aligns the onChange function so that the event.target.files is accurately adjusted on removal and allows a user to access the FileList.

Where should the reviewer start?

FileInput.js

What testing has been done on this PR?

Tested with Storybook. Updated the FileInput/Multiple story to console.log(event.target.files).

How should this be manually tested?

Storybook

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

Included link to a resource I looked into. Also, this method of using dispatchEvent can be seen in other Input components (see MaskedInput, Select, RangeInput).

What are the relevant issues?

#6056

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes, need to outline that event.target.files will correspond to the most recent event/action.

Should this PR be mentioned in the release notes?

Yes.

Is this change backwards compatible or is it a breaking change?

Compatible.

…files

… fix/fileinput-6056

amandadupell · 2022-05-18T20:16:30Z

src/js/components/FileInput/FileInput.js

+      // since inputRef.current.files is a read-only FileList
+      // https://stackoverflow.com/a/64019766
+      /* eslint-disable no-undef */
+      const dt = new DataTransfer();


this is used to store the new files object so that we're able to set it to input.current when removed. we're not able to set an array to this value.

https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer

jcfilben

If I add two files at the same time and then remove one file, event.target.files is a FileList containing both files. I was expecting there to only be one file in the FileList.

src/js/components/FileInput/FileInput.js

amandadupell · 2022-05-18T23:24:09Z

If I add two files at the same time and then remove one file, event.target.files is a FileList containing both files. I was expecting there to only be one file in the FileList.

This is what I'm seeing when I do that:
https://user-images.githubusercontent.com/38971752/169171171-50bd40f1-bc02-4444-9eb9-e161015ba86d.mov

amandadupell · 2022-05-19T21:36:57Z

after talking with jessica, we discussed the difference with event.target.files and our files array we allow users to access. with this PR, event.target.files is tied to the most recent event as it relates to the input component.

for example, each time a user goes to add files to the input, event.target.files will reflect the most recent event/action. removing will then correlate to this new event as well. this will differ from just accessing the files array.

jcfilben · 2022-05-19T21:50:50Z

after talking with jessica, we discussed the difference with event.target.files and our files array we allow users to access. with this PR, event.target.files is tied to the most recent event as it relates to the input component.

for example, each time a user goes to add files to the input, event.target.files will reflect the most recent event/action. removing will then correlate to this new event as well. this will differ from just accessing the files array.

I like this direction, it makes sense to me that event.target.files is only showing files related to the event, since we already have access the full list of files through files.

My only uncertainty is when we remove files whether event.target.files should reflect the updated file list (this is currently how the PR is handling it) or should it contain the file or files that were removed since that is the file or files tied to the event action?

@taysea, @ericsoderberghp any opinions on this?

JackGoldsworth · 2022-05-19T22:26:15Z

@jcfilben As the original reporter of this issue, I expected the event.target.files list to contain the updated file list. So if you have a list of 5 files and remove one of them, you would receive a list of the 4 remaining files.

amandadupell · 2022-05-20T15:21:05Z

@jcfilben As the original reporter of this issue, I expected the event.target.files list to contain the updated file list. So if you have a list of 5 files and remove one of them, you would receive a list of the 4 remaining files.

With this PR, when you upload 5 files and then remove one of them, event.target.files would be the remaining 4 files. If you remove another one, event.target.files would be the remaining 3 files and so on. However, if you upload 3 more files, the event would change and event.target.files would be those 3 added files. In this way, event.targe.files would be tied to the most recent input event, differing from accessing { files }.

JackGoldsworth · 2022-05-20T16:13:15Z

@amandadupell Fair enough. This brings up another issue however, which is the fact that you can't access { files } in typescript (Unless I'm doing something wrong). Should I create another issue for this?

ericsoderberghp

Overall, I like the direction this is heading. Just had a few small comments.

ericsoderberghp · 2022-05-20T19:41:49Z

src/js/components/FileInput/FileInput.js

+          window.HTMLInputElement.prototype,
+          'files',
+        ).set;
+        nativeInputValueSetter.call(inputRef.current, dt.files);


Shouldn't we be doing this irregardless of whether onChange is provided? That way a test program could introspect the DOM to get the current value.

i can move this out of the if statement based on onChange

src/js/components/FileInput/FileInput.js

… fix/fileinput-6056

amandadupell · 2022-05-20T20:37:35Z

@amandadupell Fair enough. This brings up another issue however, which is the fact that you can't access { files } in typescript (Unless I'm doing something wrong). Should I create another issue for this?

Thank you for pointing this out! Please create a new issue covering this and include a codesandbox that reproduces the error you're seeing in Typescript. Someone will pick this up from out backlog moving forward.

JackGoldsworth · 2022-05-20T23:51:32Z

@amandadupell Done. the issue can be found below if anyone is immediately interested.
#6143

amandadupell · 2022-06-02T15:50:53Z

@ericsoderberghp i spoke with jessica yesterday about aligning this PR with the native html <input type="file"> behavior. in working on this and researching a bit more, there isn't any defined behavior for removing files anf how event.target.files should be updated. ultimately, it's left up to the developer to decide on how to best remove files one by one. the only "defined" behavior is when a user clears all of the files in the input, in which event.target.files should be set to an empty string. this is currently how we handle removing all of our files or reaching an empty array of files.

https://stackoverflow.com/questions/1703228/how-can-i-clear-an-html-file-input-with-javascript
https://stackoverflow.com/questions/3144419/how-do-i-remove-a-file-from-the-filelist

considering this, i would be interested if this PR approach is a viable solution for how we decide to handle removing a file and how event.target.files shows this. since there is no defined behavior for removing a file for the input html element, i think we would just need to clearly define how we decide to handle this so users understand what event.target.files reflects.

jcfilben

This looks good to me. I think since there isn't a native way to remove a single file, showing the remaining file list seems like a reasonable behavior for event.target.files when we remove a file.

adupell added 3 commits May 18, 2022 15:53

adding logic to align onChange event to allow access to event.target.…

878b534

…files

Merge branch 'master' of https://github.com/amandadupell/grommet into…

0b6fb11

… fix/fileinput-6056

revert yarn

c42af6e

amandadupell commented May 18, 2022

View reviewed changes

amandadupell requested review from taysea and jcfilben May 18, 2022 20:19

jcfilben reviewed May 18, 2022

View reviewed changes

src/js/components/FileInput/FileInput.js Outdated Show resolved Hide resolved

combine logic

5c6c15b

ericsoderberghp reviewed May 20, 2022

View reviewed changes

adupell added 3 commits May 20, 2022 16:04

reorganizing content out of onChange conditional

02227b9

Merge branch 'master' of https://github.com/amandadupell/grommet into…

e5d7d40

… fix/fileinput-6056

removing some comments

9a54532

jcfilben approved these changes Jun 3, 2022

View reviewed changes

ericsoderberghp approved these changes Jun 6, 2022

View reviewed changes

Merge branch 'master' into fix/fileinput-6056

6606fbe

ericsoderberghp merged commit fc35dc0 into grommet:master Jun 6, 2022

ericsoderberghp linked an issue Jun 6, 2022 that may be closed by this pull request

FileInput not updating when file removed #6056

Closed

Enhance FileInput to Update event.target.files on Remove #6136

Enhance FileInput to Update event.target.files on Remove #6136

Uh oh!

Conversation

amandadupell commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

amandadupell May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcfilben left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amandadupell commented May 18, 2022

Uh oh!

amandadupell commented May 19, 2022

Uh oh!

jcfilben commented May 19, 2022

Uh oh!

JackGoldsworth commented May 19, 2022

Uh oh!

amandadupell commented May 20, 2022

Uh oh!

JackGoldsworth commented May 20, 2022

Uh oh!

ericsoderberghp left a comment

Choose a reason for hiding this comment

Uh oh!

ericsoderberghp May 20, 2022

Choose a reason for hiding this comment

Uh oh!

amandadupell May 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amandadupell commented May 20, 2022

Uh oh!

JackGoldsworth commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amandadupell commented Jun 2, 2022

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amandadupell commented May 18, 2022 •

edited

Loading

amandadupell May 18, 2022 •

edited

Loading

jcfilben left a comment •

edited

Loading

JackGoldsworth commented May 20, 2022 •

edited

Loading