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

`File.Select.files` fails on iOS 12 browsers. #4

Closed
kyasu1 opened this Issue Jan 23, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@kyasu1
Copy link

kyasu1 commented Jan 23, 2019

The examples using File.Select.files fail to call its callback on iOS 12 browsers. With my iPhone Xs/5s, both 'Take Photo' and choosing from 'Photo Library' did not work at all. While with my iPad Pro, 'Take Photo' did not work but choosing from 'Photo Library' worked.

I think this issue is related to the problem reported on this Discourse topic. Applying the fix suggested at the last post (moving the node variable declaration to the outer scope) to the generated JavaScript file seems to solve the problem.

This API is really useful when combining with elm-ui, hope this will be resolved soon !

@charukiewicz

This comment has been minimized.

Copy link

charukiewicz commented Jan 30, 2019

Confirming the same issue.

Testing using Safari on an old iPhone 6 (iOS v10), both 'Take Photo' and 'Photo Library' fail every time.

On Chrome 71 on my Android (version 8.0.0), 'Use Camera' seems to be mostly reliable (failing occasionally), and 'Select File' seems to be 100% reliable.

@charukiewicz

This comment has been minimized.

Copy link

charukiewicz commented Jan 30, 2019

I just did some testing based off of what @evancz suggested in the Discourse topic, to quote him:

Can someone test this with just JS code to see:

  1. If the bug reproduces with the exact same JS and no Elm code.
  2. If so, does adding it to the DOM and then removing it in the event handler make it more reliable?

The existing version of the Elm.Kernel.File function is:

function _File_uploadOne(mimes)
{
	return __Scheduler_binding(function(callback)
	{
		var node = document.createElementNS('http://www.w3.org/1999/xhtml', 'input');
		node.setAttribute('type', 'file');
		node.setAttribute('accept', A2(__String_join, ',', mimes));
		node.addEventListener('change', function(event)
		{
			callback(__Scheduler_succeed(event.target.files[0]));
		});
		node.dispatchEvent(new MouseEvent('click'));
	});
}

However, I made a quick-and-dirty modification to make the function append the node to the DOM and then remove it:

function _File_uploadOne(mimes)
{
    return _Scheduler_binding(function(callback)
    {
        var node = document.createElement('input');
        node.setAttribute('type', 'file');
        node.setAttribute('accept', A2(elm$core$String$join, ',', mimes));
        node.addEventListener('change', function(event)
        {  
            document.body.removeChild(node); // node is removed after change event fires
            callback(_Scheduler_succeed(event.target.files[0]));
        });   
        document.body.appendChild(node); // node is appended before click event is dispatched
        node.dispatchEvent(new MouseEvent('click'));
    });   
}

This change fixes the issue on Safari on iOS 10 (fixing both 'Take Photo' and 'Photo Library'). This continues to work on Chrome on Android, and Chrome on Linux without any issue.

Edit: It turns out that Browser.Elm.Kernel.Debugger uses the exact same technique for its own upload function.

One of the concerns that Evan mentions is modifying the DOM may cause issues with the virtual-dom:

I do not really like (2) because I do not want to mess with the DOM and potentially disrupt virtual-dom. The click should be synchronous, so it may work alright. Not super confident in that approach without testing it out a bunch.

One alternative might be to append the temporary element to the outside of the <body> by using document.documentElement.appendChild() instead of document.body.appendChild() as in my example above. This places the <input> element outside of the <body> and right before the closing </html> tag at the end of the document, if this helps mitigate the risk of interfering with the virtual-dom.

evancz added a commit that referenced this issue Jan 30, 2019

Attempt to make File.Select.files more reliable
Instead of making a new node each time, have one node that is reused
again and again. It still is not added to the DOM, but it now will not
be possible to GC it too early (assuming that was the root problem!)

Relevant to #4
@evancz

This comment has been minimized.

Copy link
Member

evancz commented Jan 30, 2019

This should be fixed in 1.0.2 which uses the same backing <input> node for each command. Assuming the root problem is that some browsers GC even though the function may still be called, this resolves that by keeping the <input> alive in the global scope of the program.

Based on the GC theory, I made a program that will allocate a bunch of models very quickly to try to get GC to run more often. The result was:

  • Example 1.0.1 which fails pretty reliably in Safari 12, especially if you wait a bit on the file selection dialog.
  • Example 1.0.2 which I could not get to fail in Safari 12 or FireFox 64.

Thanks for the report, and thank you @charukiewicz for sharing some ideas on this on discourse! If you try this out in other browsers, please share the results on the discourse thread: https://discourse.elm-lang.org/t/cross-browser-compatibility-fix-for-file-select-file-s-in-elm-file/3060/5

@evancz evancz closed this Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment