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
Core: Drop workarounds for unsupported browsers #267
Conversation
@@ -509,18 +509,6 @@ setDocument = Sizzle.setDocument = function( node ) { | |||
return !div.getElementsByTagName("*").length; | |||
}); | |||
|
|||
// Check if getElementsByClassName can be trusted | |||
support.getElementsByClassName = rnative.test( doc.getElementsByClassName ) && assert(function( div ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't drop the rnative
test. This avoids oldIE attempting to call a method that doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we're lacking tests here then since the test suite passes in oldIE on my branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors of that nature have been hard to pin down in tests, which is why we've adopted the style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it works because in both cases where getElementsByClassName
is used there's a check if it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that kind of check has shown to be problematic. In jQuery, we use typeof
. In Sizzle, we use rnative
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we don't support getElementsByClassName
polyfills.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of, looks like some jQuery flavor made it into the CLASS
find method (typeof). Let's change that to use rnative
as well for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
Chrome messes with our sorting algorithm. Chrome assumes duplicates are sorted, but we want to remove them. |
The problem is not limited to old Chrome versions then, is it? |
It's historically been a webkit issue, but iirc, it is now a sorting (de)optimization limited to Chrome. |
I mean, if you're seeing it in Chrome latest, then yes, it is still there. |
PR updated. |
} | ||
}; | ||
// Support: IE<9 (getElementsByClassName check) | ||
Expr.find["CLASS"] = ( support.getElementsByClassName = rnative.test( doc.getElementsByClassName ) ) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the support.getElementsByClassName
assignment remain up with the other support checks so we have a single place to look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK; I moved it here to be consistent with the rest of similar cases, e.g. support.qsa
is set inline. I'll move it back.
Changes applied. |
return div.getElementsByClassName("i").length === 2; | ||
}); | ||
// Support: IE<9 | ||
support.getElementsByClassName = rnative.test( doc.getElementsByClassName ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant docElem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, I guess either works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if there's a browser that has it on document but not elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left what was here before so it should work. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okies
One test could be dropped. There is also a
detectDuplicates
one that claims is for Chrome before version 14 but even in current Chrome this test returnsfalse
(true
in other browsers). What's going on here?cc @gibson042