Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

fix: handle null file types in image check #254

Merged
merged 2 commits into from
Nov 22, 2018
Merged

fix: handle null file types in image check #254

merged 2 commits into from
Nov 22, 2018

Conversation

ckotzbauer
Copy link
Contributor

Suggested merge commit message (convention)

fix: handle null file types in image check


Additional information

This PR fixes JS errors if files with null filetypes or null-files are processed by the "isImageType" function. I cannot reproduce such cases locally, but our customers can do this with D&D in the newest Chrome...

@coveralls
Copy link

coveralls commented Nov 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 18b39cf on SamhammerAG:master into 25be8e0 on ckeditor:master.

@Reinmar
Copy link
Member

Reinmar commented Nov 21, 2018

I feel unsafe about this change being made in a util. It may hide some bigger issues if this util will be used in more places. I'd still not be ideal, but I'd move this code directly to the place where isImageType() is used. Most likely you mean this place:

const images = Array.from( data.dataTransfer.files ).filter( isImageType );

Also, this seems to be a workaround for some Chrome bug, so that's why it'd be better to keep this change more locally.

@Reinmar
Copy link
Member

Reinmar commented Nov 21, 2018

BTW, any chance that you could find out more about how and when this issue occurs? It's a bit odd one :|

@ckotzbauer
Copy link
Contributor Author

Yes, I mean this place with the datatransfer.
We currently try to find out more about this issue from our users. Hopefully we get some useful infos.

@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2018

Do you think you could update the PR this week? We're finishing an iteration now and we would include it in the release.

@ckotzbauer
Copy link
Contributor Author

@Reinmar Done, I moved the null check to the dnd-logic.

@Reinmar
Copy link
Member

Reinmar commented Nov 22, 2018

Thanks! It looks great.

@Reinmar Reinmar merged commit 2a45481 into ckeditor:master Nov 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants