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

tawago commented 2 years ago

DropZone

The validation error dragErrorOverlay for dragenter event relies on accept property or customValidator function property of <DropZone>https://github.com/Shopify/polaris/blob/b541f54498462ce513b9903dfcfcb966f43e0038/polaris-react/src/components/DropZone/DropZone.tsx#L190-L210
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. https://github.com/Shopify/polaris/blob/b541f54498462ce513b9903dfcfcb966f43e0038/polaris-react/src/components/DropZone/utils/index.ts#L30 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.

ghost commented 2 years ago

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

chloerice commented 2 years ago

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 commented 2 years ago

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[bot] commented 1 year ago

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 commented 1 year ago

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[bot] commented 11 months ago

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 commented 11 months ago

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.

Yes, still a problem.

shoenseiwaso commented 6 months ago

Coming up on the two year anniversary of this - yes, it's still relevant!

genevieveluyt commented 6 months ago

@chloerice I could give this a go, though I'd like to propose an alternate solution (similar to the first proposed solution):

The System OS file picker works with both mime types and file extensions, so if I wanted my dropzone to accept say images and GLB files, I could pass an accept prop of image/jpeg,.glb. It would be great if the drag and drop validation could respect this as well, rather than requiring me to write custom logic in customValidator.

If there is a value in the accept prop that starts with '.' I think it's safe to assume that the user intends to validate against the file extension. We also know that during the dragenter and dragover events, the file name is not known:

Note: The files property of DataTransfer objects can only be accessed from within the drop event. For all other events, the files property will be empty — because its underlying data store will be in a protected mode. Ref

So it is not possible for us to validate by file extension on these events. So only in this case (if the accept prop contains a file extension), we could skip validation on dragenter and dragover events because we are missing information to validate properly, and only validate on drop events at which point the filename is known.

I think it would be worth some docs to recommend using mime types if possible (eg. if they are common mime types), else use file extension but note that on-drag validation is not supported in that case.

shoenseiwaso commented 5 months ago

@genevieveluyt @chloerice thanks for addressing this! Has the fix been merged and/or released yet?

genevieveluyt commented 5 months ago

Sorry closed it by accident! PR is here: https://github.com/Shopify/polaris/pull/12135

shoenseiwaso commented 5 months ago

Sorry closed it by accident! PR is here: #12135

All good, thanks for picking this up!

Shopify / polaris