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

Handlers are not removed while set to `undefined` on update #1804

Closed
uNmAnNeR opened this Issue Apr 17, 2017 · 14 comments

Comments

Projects
None yet
4 participants
@uNmAnNeR
Copy link

uNmAnNeR commented Apr 17, 2017

Hello!

On "Log" step, onclick is set. But on "Dont log" step onclick is set to undefined, but logging still works:
https://jsfiddle.net/hn0sunah/

I guess, this is very common problem. For example, i have common Button component, in which onclick callback could be passed, or onclick will be explicitely set to undefined.

So, may be setAttr should handle value === undefined another way, then just ignore it:
https://github.com/lhorie/mithril.js/blob/0ce5dcac3b7965174477236e02d4ac5c66f28c0e/render/render.js#L471

@isiahmeadows isiahmeadows added the bug label Apr 17, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 17, 2017

What browser is this happening in? It's not reproducing in Firefox.

@uNmAnNeR

This comment has been minimized.

Copy link

uNmAnNeR commented Apr 18, 2017

In latest Chrome, IE, and i reproduce it in FF.
I simplified example a little bit:
https://jsfiddle.net/hn0sunah/1/

Steps to reproduce:

  • unckeck "Log?"
  • click "please, no log!" link and
  • check console output. It should not logging, but it does.
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 18, 2017

@uNmAnNeR Good catch!

Removing the typeof value === "undefined" check doesn't break any test, but I'm wary of just removing it without understanding why it was there in the first place.

The check was present in the original rewrite code, paired with the in check in the remove attrs loop in updateAttrs...

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 18, 2017

@pygy It would affect cases like this:

m("div", {onclick: undefined}, "contents")

IMHO the current behavior is very buggy, because it fails to address a couple key things:

  • Event handler was added, then set to undefined (but still exists)
  • Attribute was added, then set to undefined (but still exists)

The proper fixes would be to:

  1. Change this to check attrs[key] == null instead of !(key in attrs).
  2. Dispatch for all different attribute types.
@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 28, 2017

@isiahmeadows actually, a strict undefined check would be better because null has a special meaning in attribute/properties context (working on this while working on the #1595 fix).

What do you mean by "dispatch for all different attribute types"?

Edit: Oooh, maybe you mean that this branch should be mirrored in the removal loop?
(key in element && !isAttribute(key) && ns === undefined && !isCustomElement(vnode))

There's probably a textarea bug hiding in there...

Edit2: yes, there's a textarea bug and an input bug

pygy added a commit to pygy/mithril.js that referenced this issue May 28, 2017

pygy added a commit to pygy/mithril.js that referenced this issue May 28, 2017

pygy added a commit to pygy/mithril.js that referenced this issue May 28, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 29, 2017

@pygy Most (if not all) DOM-related properties and arguments with relevant null semantics coerce undefined assignments to null, as specified by WebIDL for their nullable types (and other relevant places throughout that spec).

Just thought I'd point that out. (That's also where the == null recommendation came from.)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 29, 2017

@pygy

What do you mean by "dispatch for all different attribute types"?

Edit: Oooh, maybe you mean that this branch should be mirrored in the removal loop?

Yes, but that's only part of it - this case should be calling a helper mirroring setAttr entirely.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 29, 2017

Regarding ==null, I may have gotten the wrong impression... .value = null sets value to "" and resets select.selectedIndex to -1, even if there's an option with "" for value, whereas .value = undefined sets the value to "undefined" (for input, select and textarea)... innerHTML works the same way, and IIRC @barneycarroll sometimes uses it as an attr instead of m.trust.

That being said in both cases, the distinction looks like a wart, and I'd be surprised if anyone used the auto-conversion of undefined to string for anything useful. So 👍 on == null unless there is a really problematic case, and 👍 on mirroring.

@pygy pygy referenced this issue May 29, 2017

Merged

Re-fix #1595 #1862

pygy added a commit to pygy/mithril.js that referenced this issue May 30, 2017

pygy added a commit to pygy/mithril.js that referenced this issue May 30, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 30, 2017

@isiahmeadows so far, there's only one place where there's a difference between null and a lack of value: the original render of a select. If it's value isn't set, it will default to the first option, if set to null it will have a selectedIndex of -1.

It makes it impossible to plainly conflate null/undefined/lack of property, and makes the code more complex... (see #1865)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 31, 2017

@pygy

For those special cases (input.value, select.value), I'd handle those separately and independently (as is done now), since they're already outliers.

If it's value isn't set, it will default to the first option, if set to null it will have a selectedIndex of -1.

I mean the difference between these two, not the (in)existence of a property:

m("select", {value: undefined}, [])
m("select", {value: null}, [])

(That distinction is what I was referring to throughout, in case that was missed.)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Jun 15, 2017

Just realized this is a dupe of #1613...closing that in favor of this, since this one is more clearly detailed.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 23, 2017

Should be fixed now as of #1949. (I had a previous comment, but I wasn't sure whether #1865 did anything event-related that mine didn't.)

@fent

This comment has been minimized.

Copy link

fent commented Sep 25, 2017

Just ran into this while updating to v1.0.0 with boolean-type attributes, such as disabled.

In v0.2.5, mithril removed attributes if they were undefined, but now it leaves them.

Here's an example https://codepen.io/anon/pen/qPRRPZ?editors=1011

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 26, 2017

@fent To clarify, it should be fixed in next, but the fix would have to be backported to v1.

pygy added a commit to pygy/mithril.js that referenced this issue May 31, 2018

pygy added a commit to pygy/mithril.js that referenced this issue May 31, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Jun 1, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Jun 1, 2018

@pygy pygy closed this in #1865 Jun 1, 2018

pygy added a commit that referenced this issue Jun 1, 2018

pygy added a commit that referenced this issue Jun 1, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this issue Oct 12, 2018

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