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

can.observe.delegate fails on compound selectors with wildcards #119

Closed
rjgotten opened this Issue Oct 18, 2012 · 2 comments

Comments

Projects
None yet
3 participants
@rjgotten

rjgotten commented Oct 18, 2012

Given a compound selector with a wildcard in it used in an event binding to a can.Observe instance:

ob.delegate( "foo.* bar", "set", function( ob, event, newVal, oldVal, prop ){  })

When any child attribute of "foo" is changed (let's say "foo.baz", for instance), eventually can.observe.delegate will hit line 94, which checks:

valuesEqual = this.attr(attr.attr) !== undefined

As the obseve will never have a property named "foo.*", valuesEqual will be false and the bound event handler will never fire.

Imho this entire test should be removed:

else if (valuesEqual && delegate.attrs.length > 1){
  // if there are multiple attributes, each has to at
  // least have some value
  valuesEqual = this.attr(attr.attr) !== undefined
}

Why is it checking if all attributes have an assigned value when the documentation clearly states that a selector like "foo bar" matches changes to "foo" or "bar" ? If either is valid, the bound handler should fire. Period.

[EDIT]
Hmm.. From the looks of it, the entire value checking logic is busted as well: it assumes 'AND' semantics instead of 'OR' semantics. E.g. for a selector "foo=a bar=b", if "foo" is set to "a", while "bar" is not equal to "b" the selector will not match...

@AnthonyMann

This comment has been minimized.

Show comment
Hide comment

AnthonyMann commented Oct 24, 2012

+1

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Nov 7, 2012

Contributor

We will heavily consider this during the next release, but for 1.1 it will stay as is.

Contributor

imjoshdean commented Nov 7, 2012

We will heavily consider this during the next release, but for 1.1 it will stay as is.

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