Follow-up for #8908 fix #1036

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
Member

markelog commented Nov 18, 2012

@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.

Owner

dmethvin commented Nov 19, 2012

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?

Member

markelog commented Nov 19, 2012

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.

Contributor

elijahmanor commented Nov 19, 2012

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/

Member

markelog commented Nov 19, 2012

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...

Owner

gnarf commented Nov 25, 2012

This needs a rebase

Member

markelog commented Nov 25, 2012

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

@gnarf gnarf and 3 others commented on an outdated diff Nov 25, 2012

src/css.js
@@ -196,6 +196,12 @@ jQuery.extend({
value += "px";
}
+ // Fixes #8908, it can be done more correctly by specifing setters in cssHooks,
+ // but it would mean to define eight (for every problematic property) identical functions
+ if ( value === "" && ~name.indexOf("background") ) {
@gnarf

gnarf Nov 25, 2012

Owner

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

cc @rwldrn @dmethvin to confirm

@rwaldron

rwaldron Nov 26, 2012

Member

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

@dmethvin

dmethvin Nov 26, 2012

Owner

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

rwaldron Nov 26, 2012

Member

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

markelog Nov 26, 2012

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

and it saves two bytes... done

@gnarf gnarf commented on an outdated diff Nov 25, 2012

test/unit/css.js
+ ok( true, style.name + ": style isn't supported and therefore not an issue" );
+ ok( true );
+ return true;
+ }
+
+ $source.css( style.name, style.value[ 0 ] );
+ $children.css( style.name, style.value[ 0 ] );
+
+ $clone = $source.clone();
+ $clonedChildren = $clone.children();
+
+ $clone.css( style.name, "" );
+ $clonedChildren.css( style.name, "" );
+
+ window.setTimeout(function() {
+ ok( ~jQuery.inArray( $source.css( style.name ), style.value ),
@gnarf

gnarf Nov 25, 2012

Owner

There are two uses of ~ here instead of !== -1 as well. I know they were pre-existing, but we should probably fix these...

Owner

gnarf commented Nov 25, 2012

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

Member

markelog commented Nov 26, 2012

@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.

Member

markelog commented Nov 26, 2012

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

Sorry, not me, @elijahmanor did.

Member

mikesherov commented Nov 26, 2012

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

Owner

dmethvin commented Dec 12, 2012

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

Member

markelog commented Dec 12, 2012

@Orkel can you rebase?

done

dmethvin closed this in 643ecf9 Dec 13, 2012

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