DropZone always returns error when glb/gltf file triggers `dragenter` event

[tawago]

DropZone

• The validation error dragErrorOverlay for dragenter event relies on accept property or customValidator function property of <DropZone>

  polaris/polaris-react/src/components/DropZone/DropZone.tsx

  Lines 190 to 210 in b541f54

  	const getValidatedFiles = useCallback(
  	(files: File[] | DataTransferItem[]) => {
  	const acceptedFiles: File[] = [];
  	const rejectedFiles: File[] = [];
  	Array.from(files as File[]).forEach((file: File) => {
  	!fileAccepted(file, accept) ||
  	(customValidator && !customValidator(file))
  	? rejectedFiles.push(file)
  	: acceptedFiles.push(file);
  	});
  	if (!allowMultiple) {
  	acceptedFiles.splice(1, acceptedFiles.length);
  	rejectedFiles.push(...acceptedFiles.slice(1));
  	}
  	return {files, acceptedFiles, rejectedFiles};
  	},
  	[accept, allowMultiple, customValidator],
  	);

• When accept property is present and its !fileAccepted returns true, customValidator never gets evaluated.

• fileAccepted always returns false for glb/gltf file because:

  1. When a file is dragentered, it is not a File instance but DataTransferItem instance and DataTransferItem only has kind and type properties. When a glb/gltf dragentered, however,DataTransferItem's type has an empty string; thus an-always validation error. This part is related to a situation when accept property is a mimeType string like model/gltf+json,model/gltf-binary instead of a dot string like .glb. The validation works for files like video/mp4 because DataTransferItem has a proper type.

  2. When accept property is a dot string like .glb,.mp4,.wav, fileAccepted always returns false because DataTransferItem does not have name property.

     polaris/polaris-react/src/components/DropZone/utils/index.ts

     Line 30 in b541f54

     const fileName = file.name || '';

     This is a bug that should be fixed separately. all file types not working.

  3. Additionally, File instance of drop event has type property but it is an empty string. name gets correct information tho. file.name validation would work only when accept is a dot string.

• To avoid dragErrorOverlay showing up for dragenter event, a file validation needs to rely on customValidator and omit accept property

• This compromises "click and select files" user interaction of System UI outside of browsers because to filter file types on System, it relies on accept property of <input type="file"> tag.

Examples

dragErrorOverlay showing up even tho DropZone actually accepts glb

const [files, setFiles] = useState<File[]>()
const handleDrop = (acceptedFiles: File[]) => setFiles(acceptedFiles)
const fileUpload = !files.length && <DropZone.FileUpload />
const uploadedFiles = files.length > 0 && (
    <Stack vertical>
      {files.map((file, index) => (
        <Stack alignment="center" key={index}>
          <Thumbnail
            size="small"
            alt={file.name}
            source={window.URL.createObjectURL(file)}
          />
          <div>
            {file.name} <Caption>{file.size} bytes</Caption>
          </div>
        </Stack>
      ))}
    </Stack>
);
...
      <DropZone accept="model/gltf-binary" type="file" onDropAccepted={handleDrop}>
        {uploadedFiles}
        {fileUpload}
      </DropZone>

Best practices

The DropZone component should either:

• skip fileAccepted validation for dragenter event when DataTransferItem.type has an empty string. (with a better documentation otherwise coders implicitly have to use a dot string for accept prop)
• prioritize customValidator over fileAccepted and give coders a better control. If customValidator is present, just skip fileAccepted. (This is because I would rather have a mimetype <=> a dot string extension conversion in the customValidator and check both file.name and file.type with a specified filetypes as accept prop)

-ps
I would like to create a PR but I wanted to make sure that we have a consensus on which solution would be better.

👋 Thanks for opening your first issue. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

[chloerice]

Hey @tawago 👋🏽 Thanks for flagging and investigating this bug! I think solution B is the way to go, the customValidator method should be prioritized, especially since it's a required prop 🧐 If you run into any blockers or questions raise them here and we'll figure it out together, and feel free to ping me for review in your PR 😁 🙏🏽

[laurkim]

Hey @tawago, just doing some backlog cleanup and noticed you mentioned opening a PR and @chloerice gave feedback on which solution would be preferable. Going to add you as an assignee to this issue 😃

[github-actions]

Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

[shoenseiwaso]

Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

Still relevant, thanks!

[github-actions]

Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.