[DEPENDENCY HOLD] Almost fix #13539: use Sizzle.selectors.attrHandle instead of overriding Sizzle.attr #1215

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
Member

gibson042 commented Mar 29, 2013

Don't commit until the Sizzle dependency lands, but the numbers speak for theirselves:

   raw     gz Sizes                                                             
270935  80384 dist/jquery.js                                                    
 93054  32735 dist/jquery.min.js                                                

   raw     gz Compared to 1.9-stable @ e3a802cbf7d66d88e3659ad344bce86b99d029be 
   -16   -272 dist/jquery.js                                                    
   -47   -106 dist/jquery.min.js

Almost half of the savings are independent of Sizzle changes, but 64 bytes off of jquery.min.js is still nothing to sneeze at. I'll make an analog for 2.x after this one is cleared.

@gibson042 gibson042 referenced this pull request in jquery/sizzle Mar 29, 2013

Closed

Base for jQuery #13539: more robust Sizzle.attr #200

test/unit/traversing.js
- ok( !jQuery(window).is("a"), "Checking is on a window does not throw an exception" );
- ok( !jQuery(document).is("a"), "Checking is on a document does not throw an exception" );
+ ok( jQuery(window).is("a") || true, "Checking is on a window does not throw an exception" );
+ ok( jQuery(document).is("a") || true, "Checking is on a document does not throw an exception" );
@timmywil

timmywil Mar 29, 2013

Owner

These tests should not change. Windows and documents are not anchors.

@gibson042

gibson042 Mar 29, 2013

Member

True, but these are only testing that no exception is thrown. We shouldn't make guarantees about results from invalid input.

@timmywil

timmywil Mar 29, 2013

Owner

Can't we supply both like we were before? Arguably jQuery(window).is(anything) is valid input, but it's always false.

@timmywil

timmywil Mar 30, 2013

Owner

I'm confused. What's the point?

@gibson042

gibson042 Mar 30, 2013

Member

That we're free to let jQuery( window ).is("a") return true at some future point (unlikely as that may be), just like jQuery( window ).is("*") returns true today. The explicit purpose of these tests is to verify lack of exception, and they shouldn't do more than that unless we commit to having non-elements fail all selectors (which, incidentally, is established on the side by this PR). If we are ready for that, I can expand them rather than restrict their scope.

@scottgonzalez

scottgonzalez Mar 30, 2013

Owner

jQuery( window ).is( "*" ) should return false. Returning true should be considered a bug. Having non-elements fail all selectors is fine, since that's correct behavior.

I feel like we've had this discussion a few times, but the general concept is that window and document need to return sane values for all methods so that plugin authors don't have to constantly add checks and write exceptions around the various types of input.

@timmywil

timmywil Mar 30, 2013

Owner

Agreed Scott. I remember having this discussion as well; that was the decision that was made. We need to ensure consistency here.

@gibson042

gibson042 Mar 30, 2013

Member

I was sold basically as soon as I hit submit. Tests updated, and jQuery is now marginally more consistent!

- q("google"),
- "Text nodes fail attribute tests without exception"
- );
-
@timmywil

timmywil Mar 29, 2013

Owner

Why was this removed?

@gibson042

gibson042 Mar 29, 2013

Member

It was moved to traversing, and there expanded.

@timmywil

timmywil Mar 29, 2013

Owner

I think that link is off, but I trust you. I didn't know we moved it.

- ok( jQuery(window).is(":visible"), "Calling is(':visible') on window does not throw an error in IE.");
- ok( jQuery(document).is(":visible"), "Calling is(':visible') on document does not throw an error in IE.");
+ ok( jQuery(window).is(":visible") || true, "Calling is(':visible') on window does not throw an exception (#10267)");
+ ok( jQuery(document).is(":visible") || true, "Calling is(':visible') on document does not throw an exception (#10267)");
@timmywil

timmywil Mar 29, 2013

Owner

If this is for selector-native, it should throw an exception.

@gibson042

gibson042 Mar 29, 2013

Member

Good point; we'll have to deal with that in the 2.x cherry-pick.

Owner

timmywil commented Mar 29, 2013

I see, so you want jQuery to lean more on its selector engine for attribute handling. This will make it more difficult to trade out selector engines, and while this saves space on the 1.x branch, I imagine if we want to keep selector-native going, we'll end up duplicating attr code in 2 or even 3 places.

Member

gibson042 commented Mar 29, 2013

I think it'll just be in one place, with selector-native defining a minimal attrHandle hook for us to override boolean handling. Which will be necessary if we change selector engines anyway, because (for example) native operations in modern browsers return unassigned boolean attributes as empty string.

Owner

timmywil commented Mar 29, 2013

Well, that would be 2 places.

Owner

timmywil commented Mar 29, 2013

As in the number of places maintained, not the number in one build.

gibson042 added a commit that referenced this pull request Apr 5, 2013

@gibson042 gibson042 closed this in 5d1dfe7 Apr 5, 2013

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

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