Choose file button opens file selector in first visible attachment input on page in Chrome

[asgerb]

Hey hey Tomas, I noticed a bug recently when having multiple attachment inputs open on a page (Chrome, haven't tried other browsers yet).

What happens is that clicking the "Choose file" button (or on the containing label element) triggers the very first attachment file input on the page. So when you click and select a file in any input but the first, the file gets added to the very first file input on the page.

The problem is that the input's id is the same for all inputs: attachment_file. The label's for attribute is therefore also attachment_file across all labels. From an html standpoint this is also not valid – every id should be unique – but that is a problem in rails forms in general.

I think the best way to solve this would be to amend the attachment_file_field method and give the file input a more specific id (see below), but I wanted to get your thoughts on it first...

My solution doesn't solve the situation where we have two "identical" attachment inputs open (meaning with similar parent/child relations, like two image modules for instace), where the bug would occur as well, seeing as the id would be the same for both inputs. One way to solve that would be to make the input's id field contain the id of the parent.

# lib/simple_form_attachments/attachment_input.rb, line 185
def attachment_file_field
  input_html_options = {
    multiple: multiple?,
    accept: accepted_file_types,
    class: 'file',
    id: "#{parent_name}_#{relation_key}_file"
  }
  template.label_tag(input_html_options[:id) do
    template.file_field_tag('attachment[file]', input_html_options) +
    template.content_tag(:span, I18n.t(:choose_file, scope: 'simple_form_attachments.links', count: (multiple? ? 2 : 1)), class: SimpleFormAttachments.dom_class(:label, [:choose_file]))
  end
end

[tomasc]

Hi @asgerb I understand the issue I think.

How about making a branch, and adding the problematic setup to the test app (/test/dummy) so we can see it and find a solid solution? (I have a hunch that something along the lines what you propose is what we'll end up with.)

[asgerb]

I'm not currently allowed to push to the repo, should I make my own fork?

[tomasc]

Sorry, updated the permissions so you should be able to work within this repo.

[asgerb]

Great, pushed a test case to multiple-file-inputs-bug

[tomasc]

Thank you, will check it out later today.

[tomasc]

@asgerb I get error when booting up the app and opening at http://127.0.0.1:3000. Do you happen to know what am I doing wrong? Thanks!

[asgerb]

Hmmm, not sure what the problem is, runs fine here – what's the error?

[tomasc]

TestsController#new is missing a template for this request format and variant.

[asgerb]

What happens if you go to http://127.0.0.1:3000/tests?

[tomasc]

Ah my bad, I removed the extra Gemfile and that caused the issue. I pushed the update to your branch.

[tomasc]

I see and remember running into this as well. I found these:

https://github.com/blueimp/jQuery-File-Upload/wiki/Multiple-File-Upload-Widgets-on-the-same-page https://github.com/blueimp/jQuery-File-Upload/wiki/Multiple-File-Input-Fields-in-One-Form

I think we should solve this for good, hmm …

[asgerb]

As mentioned I think the default way in which rails creates the ids for form fields are flawed (not sure if simple_form is doing smth different or just inheriting from rails here). They are simply doing something like "#{model_name}_#{field_name}", which obviously is no good if there are multiple instances of a model in the dom.

Anyways, I guess we just need to decide on the best way to generate unique ids. I was thinking smth like "#{parent_model_name}_#{parent_id}_#{child_model_name}_#{child_field_name}".

Then you have the case of new parents without an id... How to solve that I'm not sure.

[tomasc]

OK, I added SecureRandom to the field id. Seems to do the trick. I pushed the branch, can you test it in your real app please? Also dropping files, removing, saving form etc.?

[tomasc]

BTW we could alternatively modify the ID from the JavaScript – which might be cleaner solution?

[tomasc]

I think updating the IDs as a first thing in JS init might solve the new parents issue you mention as well.

[tomasc]

I pushed the JS solution. It does the same thing – but via JS. Take a look. Which one do you prefer?

[tomasc]

Somehow I think that the issue is caused by JS, so in a way it make sense to again solve it via JS. ?

[asgerb]

I prefer the ruby solution as I don't see how the issue is with JS. The id should ideally be unique from the get go. Even if we completely remove the JS (at least ours) from the equation, the issue remains.

[tomasc]

Yeah, but would not we have to then rewrite all Rails form generators?

The issue is caused by JS, since the jquery uploader somehow does not bind to specific element, but refers via its ID. I say let's go with the JS solution as it addresses problem that's coming out of the uploader JS. That way it is also nicely coupled with the JS that inits the file uploader.

In any case, could you please test it thoroughly in your app? Feel free to make PR then.

[asgerb]

Just did a quick codepen to show how the issue is not with the js. Try clicking on the second label (yellow element) and add a file.

[tomasc]

works as expected in both Safari and Chrome ?

[asgerb]

Great, then perhaps the whole can be solved by me updating Chrome (on version 61.0.3163.100) 😄

[asgerb]

Hmm, in both Safari and Chrome (updated) on my computer it attaches the files to the first file input when clicking the second label.

[asgerb]

[tomasc]

Ah you're right — i did not know the yellow thing is a label … Still i wonder whether we should interfere with Ruby – I guess they'll fix it eventually?

[asgerb]

Unfortunately I think it's a bit more complex. We are using template (I guess this is a wrapper for an ActionView instance?) to create all the fields, but this knows nothing of the "context", i.e. what form we are inside, who is the owner etc. @builder on the other hand is an instance of SimpleForm::FormBuilder and knows about the context. Using that might be the correct way to build the fields in a more dynamic and unique way, but it will require us to change the code quite a bit.

[tomasc]

Hmm, you might be right. Still, though, I think we'll end up with the same id. Shall we sleep in it?

[asgerb]

Yeah I'm also not sure it completely solves our issue 😢 Let's sleep on it and look at it with fresh eyes tomorrow 🙂

[tomasc]

I think no matter how we get there (Ruby or JS) it is important to just test whether these unique ids do solve the problem (and do not cause any other). If so we can flip the coin ;-).

Still I somehow think this is more of a front-end issue and would prefer to leave the Rails forms generated as they are and modify them later via JS.

But … let's sleep :-).

Thanks Asger!

[asgerb]

Hey hey Tomas, So after sleeping on it a bit I think changing the forms to use @builder will not fix the issue completely and I actually think it makes sense for the fields to be generic and only refer to attachment, instead of the actual relation. So I think we should just do it via js and keep the issue front-end as you say. Since we're only changing the id (and this is not submitted in the form), it should have no effect, but I will test it out on a few apps to make sure.

[tomasc]

Hi Asger, yes I agree. Test it out please and when ready submit a PR and let's do it! Thanks for this! T

[tomasc]

fixed via #22