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

Test and Fix #1168. Add test to prove issue with inDoc. Add check for node in inDoc #1169

Merged
merged 3 commits into from Sep 9, 2013

Conversation

Projects
None yet
4 participants
@apipkin
Copy link
Contributor

apipkin commented Sep 6, 2013

Node::inDoc wasn't checking if the node existed before it accessed a property on it. This caused an error when Node was no longer bound to a node. This fix adds a check for the node.

Fix Issue #1168

Ping @msweeney

  • Test
  • Code Fix
  • History
  • Sign Off or 3 days

apipkin added some commits Sep 6, 2013

},
'should return false if node is removed': function () {
var myNode = Y.one('*');

This comment has been minimized.

@lsmith

lsmith Sep 7, 2013

Contributor

Create a node rather than fetching one from the document. Because Node instances are cached in Node._instances, this can affect other tests.

This comment has been minimized.

@lsmith

lsmith Sep 7, 2013

Contributor

More specific: create a node, test inDoc() === false, insert it into the DOM, test inDoc() === true, call its destroy() method (rather than set _node explicitly), test inDoc() === false.

This comment has been minimized.

@lsmith

lsmith Sep 7, 2013

Contributor

And if you want to be hyper-diligent, store var el = myNode.getDOMNode(); before destroying it, and do el.parentNode.removeChild(el); afterward to leave the test env unaltered. But that's your call.

@lsmith

This comment has been minimized.

Copy link
Contributor

lsmith commented Sep 7, 2013

After the test fix, 👍

@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Sep 7, 2013

@lsmith Thanks for the notes!

return Y_DOM.contains(doc.documentElement, node);

if (node) {
doc = (doc) ? doc._node || doc : node[OWNER_DOCUMENT];

This comment has been minimized.

@ezequiel

ezequiel Sep 7, 2013

Contributor

Any reason for the parentheses around doc? They seem to be unnecessary, as it doesn't add any clarity to the expression.

This comment has been minimized.

@apipkin

apipkin Sep 7, 2013

Contributor

Not really sure on this one. My guess would be it was there for lint passing.

This comment has been minimized.

@lsmith

lsmith Sep 7, 2013

Contributor

It is a stylistic choice. Wrapping more complex ternary conditionals in parens is helpful for readability, and doing so for simple ternaries is a consistency thing. It's not harmful, at any rate.

This comment has been minimized.

@rgrove

rgrove Sep 7, 2013

Contributor

I'm not sure how it makes anything more readable in this case. If I wanted to make this statement more readable, I'd wrap the OR operation.

This comment has been minimized.

@lsmith

lsmith Sep 7, 2013

Contributor

It doesn't in this case. It's a stylistic choice, like I said. @apipkin just wrapped existing code in a new conditional, so as long as it remains functional, its style is irrelevant to this PR.

This comment has been minimized.

@ezequiel

@apipkin apipkin merged commit b2fec29 into yui:dev-master Sep 9, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment