Follow-up for #8908 fix

[markelog]

@elijahmanor, @dmethvin
Didn't mean to step on any toes here, but unfortunately fix for #8908 is incomplete

First of all, this comment is incorrect, it should 8908 not 8909, simple typo there is.
This assignment is redundant, JSHint is now ok without it.

But what more important, these lines, in my opinion, should be in cloneFixAttribute function, although function name might be confusing.

1. There is no reason to use getComputedStyle on xml nodes.
2. You would not have to check for nodeType, cloneFixAttribute already do that.
3. This fix would not fix issue like that, although, it helps for childrens of disconnected and cloned parent element, but only in IE9.

You might already talked about this, and some perfomance decrease might be an issue, but it felt important to bring this up.

[dmethvin]

Most of this looks good to me! Renaming the function does make its goal clearer. On your point 3, the bug in http://jsfiddle.net/3bAvn/ is currently fixed by this patch isn't it? Why remove the patch in css.js?

[markelog]

On your point 3, the bug in http://jsfiddle.net/3bAvn/ is currently fixed by this patch isn't it?

nope, this is a main reason why i opened this pull, but not only that test case is problem, current fix is not fixes even
cloned parent, check this in IE10.

I updated pull to address this issue too, now things are much more simpler.

[elijahmanor]

Hmm, @Orkel I'm not seeing an issue in that last jsFiddle you mentioned. I add some more console.logs and here is a screenshot in IE10 from browserstack and in a VM I have locally http://cl.ly/image/31311T392M0D via http://jsfiddle.net/3s8EH/5/

I do see the issue you mentioned above about the children having a problem. I updated your jsFiddle a little and yes, it shows the same issue about children having the same issue (as @dmethvin thought there could be an issue the other day) http://jsfiddle.net/4uK6R/

[markelog]

Hmm, @Orkel I'm not seeing an issue in that last jsFiddle you mentioned. I add some more console.logs and here is a screenshot in IE10 from browserstack and in a VM I have locally http://cl.ly/image/31311T392M0D via http://jsfiddle.net/3s8EH/5/

That's weird, indeed, browserstacks ie10 does not show it, but mine ie10 does. Judging by Wikipedia article i have last stable version...

[gnarf]

This needs a rebase

[markelog]

@gnarf37 done.
@elijahmanor could you confirm resolve of the issue?

[gnarf]

Though most of us can read the ~name.indexOf I think we prefer name.indexOf() !== -1 for readability.

cc @rwldrn @dmethvin to confirm

[rwaldron]

It's less obvious to someone unfamiliar with bitwise operators.

[dmethvin]

Yeah the bitwise op is not as obvious. In this case we're looking for strings starting with "background" right? So you could check for 0 and use !name.indexOf() but checking for -1 is probably the most obvious.

[rwaldron]

If we're looking for "background" at the beginning, then the condition should just be that: name.indexOf("background") === 0. The intention won't be mistaken and we're spared the cost of coercion (though likely negligible)

[markelog]

name.indexOf("background") === 0

and it saves two bytes... done

[gnarf]

Random question - should that test be in css or manipulation? cc @mikesherov

[markelog]

@gnarf37

Though most of us can read the ~name.indexOf I think we prefer name.indexOf() !== -1 for readability.

judging by the source, you use >= or <, but never !== -1 just like ~, personally, i dont care, gzip-size is the same.

Random question - should that test be in css or manipulation?

Well, i changed css module, so i added tests to it.

[markelog]

Well, i changed css module, so i added tests to it.

Sorry, not me, @elijahmanor did.

[mikesherov]

@gnarf37, I think it belongs in either one, but probably better in css.js for dependency sake.

[dmethvin]

Now that gh-1034 landed I tried rebasing this but it was showing conflicts I couldn't figure out offhand. @Orkel can you rebase?

[markelog]

@Orkel can you rebase?

done