Skip to content
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

Repeating DOM (input[type=file] has a ~readonly value, causing crashes) #1956

Closed
rbt opened this issue Sep 1, 2017 · 8 comments · Fixed by #2578
Closed

Repeating DOM (input[type=file] has a ~readonly value, causing crashes) #1956

rbt opened this issue Sep 1, 2017 · 8 comments · Fixed by #2578
Labels
Type: Bug For bugs and any other unexpected breakage
Milestone

Comments

@rbt
Copy link

rbt commented Sep 1, 2017

On submission of the form with a file selected, the DOM repeats. I was attempting to make a progress bar for a file upload when this appeared.

Seems to be broken in both 1.1.3 and Next in both FireFox and Chrome.

The main issue seems to be the round-trip of e.target.value for a file selection box. I've run out of brainpower to simplify this further but it almost certainly could be.

Order of the elements also seems to matter. Putting new material (version string in this case) after the form doesn't seem to cause the repeat.

https://codepen.io/rbt123/pen/WEPdby

@dead-claudia dead-claudia added the Type: Bug For bugs and any other unexpected breakage label Sep 3, 2017
@dead-claudia
Copy link
Member

After toying around, I'm finding weirder things than even that... (pen, as minimal as I can get it)

  • No wrapper element is required at all for any of them
  • It can easily be reproduced with m.render directly
  • The duplicated element need not be of any particular type; even a string works
  • The duplicated element must not be rendered on the first pass.
  • For some reason, I can't reproduce it with anything other than an input[type=file].
  • The onchange event listener must be a Mithril-known one, but it need not re-render on click.
  • The input[type=file] needs to change first, and the value "set" to whatever the new file is. (Mithril avoids setting the value when it's equal, so the spec'd error never happens.)
  • The onchange listener must not update the state if it re-renders.
  • The button click must update the state and re-render.

Super odd...

@pygy
Copy link
Member

pygy commented Sep 3, 2017

I moved it to flems for toying around with Mithril itself, and I get this error every first time I click the button after picking a file (look at the real console, flems only shows the last one that's been thrown):

InvalidStateError: Failed to set the 'value' property on 'HTMLInputElement': This input element accepts a filename, which may only be programmatically set to the empty string.

Not sure how that error ends up confusing m.render(), but it does.

Do we consider that a user error (value should not be set programmatically), or do we add it to the list of input.value exceptions?

Edit: reduced even more. onchange is a red herring...

Edit2: @rbt you can delete line 79 (inputOptions['value'] = view.fileValue;), it will solve your issue AFAICT. You need to make sure that the input[type=file] is preserved on diff (it is in your pen).

Edi3: m.render crashes before it swaps the root.vnodes field. As a consequence, the diff always starts with the initial vnode tree as old, and always adds the string to the DOM.

@dead-claudia
Copy link
Member

@pygy Good spot! (I totally overlooked that part...)

Do we consider that a user error (value should not be set programmatically), or do we add it to the list of input.value exceptions?

I'd add this as another exception, provided it's the same string. If it's different, it's a user error, but if it's still the same string, we should accept it, so it still works with the common two-way binding pattern:

m("input[type=file]", {
    value: this.value(),
    onchange: m.withAttr("value", this.value)
})

@pygy
Copy link
Member

pygy commented Sep 3, 2017

We'd also need to special case the empty string (to reset the value)...

@rbt
Copy link
Author

rbt commented Sep 18, 2017

@pygy: Yep. Prior to submitting the ticket I changed my code to check the field type and skip setting the value for "file" fields. It does indeed solve the issue.

For a Mithril fix, I'd be perfectly happy if it threw an error.

@pygy pygy changed the title Repeating DOM Repeating DOM (input[type=file] has a ~readonly value, causing crashes) Nov 23, 2017
@dead-claudia
Copy link
Member

dead-claudia commented Nov 7, 2018

@dead-claudia dead-claudia added this to the 2.0.0 milestone Nov 7, 2018
@dead-claudia
Copy link
Member

Not a blocker for v2, but it's something I want to get fixed soon.

@dead-claudia dead-claudia modified the milestones: 2.0.0, post-v2 Jul 24, 2019
@dead-claudia
Copy link
Member

The root cause is a lot more subtle than it looks: Mithril doesn't know how to deal with errors during rendering at all, much less errors from DOM manipulations. It stems from input.value's setter throws for <input type=file> when it's not set to the empty string, but you can reproduce it with just components and normal (non-throwing) DOM nodes.

This is blocked on #2273.

dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Mar 16, 2020
Note: this doesn't actually provide proper error handling. This just
kicks the can down the road by addressing the file input as a one-off
fix (since it's the only such DOM property I'm aware of that can throw
on set).
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Mar 16, 2020
Note: this doesn't actually provide proper error handling. This just
kicks the can down the road by addressing the file input as a one-off
fix (since it's the only such DOM property I'm aware of that can throw
on set).
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Mar 16, 2020
Note: this doesn't actually provide proper error handling. This just
kicks the can down the road by addressing the file input as a one-off
fix (since it's the only such DOM property I'm aware of that can throw
on set).
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Mar 16, 2020
Note: this doesn't actually provide proper error handling. This just
kicks the can down the road by addressing the file input as a one-off
fix (since it's the only such DOM property I'm aware of that can throw
on set).
@orbitbot orbitbot modified the milestones: post-v2, 2.1.0 Jul 5, 2020
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Sep 15, 2020
Note: this doesn't actually provide proper error handling. This just
kicks the can down the road by addressing the file input as a one-off
fix (since it's the only such DOM property I'm aware of that can throw
on set).
@dead-claudia dead-claudia moved this to Closed in Triage/bugs Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
Status: Closed
4 participants