Permalink
Browse files

Fix #10814. Make support tests lazy and broken out to components.

  • Loading branch information...
1 parent 776012b commit bbbdd947256a3fcd788fb9d4f306046082a1ef1f @mgol mgol committed Aug 26, 2013
View
@@ -42,7 +42,7 @@ module.exports = function( grunt ) {
ajax: [ "manipulation/_evalUrl" ],
callbacks: [ "deferred" ],
css: [ "effects", "dimensions", "offset" ],
- sizzle: [ "css/hidden-visible-selectors", "effects/animated-selector" ]
+ sizzle: [ "css/hiddenVisibleSelectors", "effects/animatedSelector" ]
}
}
},
View
@@ -88,13 +88,12 @@ Some example modules that can be excluded are:
- **exports/amd**: Exclude the AMD definition.
- **core/ready**: Exclude the ready module if you place your scripts at the end of the body. Any ready callbacks bound with `jQuery()` will simply be called immediately. However, `jQuery(document).ready()` will not be a function and `.on("ready", ...)` or similar will not be triggered.
- **deferred**: Exclude jQuery.Deferred. This also removes jQuery.Callbacks. *Note* that modules that depend on jQuery.Deferred(AJAX, effects, core/ready) will not be removed and will still expect jQuery.Deferred to be there. Include your own jQuery.Deferred implementation or exclude those modules as well (`grunt custom:-deferred,-ajax,-effects,-core/ready`).
-- **support**: Excluding the support module is not recommended, but possible. It's your responsibility to either remove modules that use jQuery.support (many of them) or replace the values wherever jQuery.support is used. This is mainly only useful when building a barebones version of jQuery.
As a special case, you may also replace Sizzle by using a special flag `grunt custom:-sizzle`.
- **sizzle**: The Sizzle selector engine. When this module is excluded, it is replaced by a rudimentary selector engine based on the browser's `querySelectorAll` method that does not support jQuery selector extensions or enhanced semantics. See the selector-native.js file for details.
-*Note*: Excluding Sizzle will also exclude all jQuery selector extensions (such as `effects/animated-selector` and `css/hidden-visible-selectors`).
+*Note*: Excluding Sizzle will also exclude all jQuery selector extensions (such as `effects/animatedSelector` and `css/hiddenVisibleSelectors`).
The build process shows a message for each dependent module it excludes or includes.
View
@@ -1,8 +1,8 @@
define([
"../core",
- "../ajax",
- "../support"
-], function( jQuery ) {
+ "../var/support",
+ "../ajax"
@staabm

staabm Jun 1, 2015

Contributor

why does this module depend on ../ajax but doesn't use a reference to the module in the callback a line below? Is this related to the buildsystem?

@mgol

mgol Jun 1, 2015

Member

This module needs jQuery.ajaxTransport to be defined and it's defined in ../ajax. We have more dependencies like that; perhaps we could break some of it further into smaller components but that requires time & careful consideration of each case.

@staabm

staabm Jun 1, 2015

Contributor

thanks for your explanation.

when the ajax module would actually be referenced in the below function( jQuery, support, ajax ) and then used as ajax.ajaxTransport this dependency would be more obvious. Also it wouldn't rely on the inner workings of ajax to attach things to the global jQuery class.

But I guess you do this kind of "trick" to save some bytes, right?

@mgol

mgol Jun 1, 2015

Member

Ah, right. Not sure why it was done this way but we rely on too many "globals" right now; some are currently being hidden in tickets like #2058 or #2224. jQuery.ajax will obviously stay a "global" but your idea seems like sth good to do.

@staabm

staabm Jun 1, 2015

Contributor

ok, will provide a PR then later this week.

@jaubourg

jaubourg Jun 1, 2015

Member

I initially created $.ajax.prefilter and $.ajax.transport but it made a lot of plugins fail because they relied on duck-punching $.ajax (effectively nullifying those two). The "fix" was to use $.ajaxPrefilter and $.ajaxTransport. So it's not about byte count, it's about compatibility with duck-punching plugins.

@staabm

staabm Jun 1, 2015

Contributor

@jaubourg this file does not define ajaxTransport but calls it. I won't change the name or namespace of any function involved.

@mgol

mgol Jun 1, 2015

Member

@staabm Right but you cannot call ajax.ajaxTransport as it won't be defined as jQuery.ajax.ajaxTransport but as jQuery.ajaxTransport. So for it to make sense you'd need to break out src/ajax.js into sub-components, one defining ajaxTransport and then you'd put it in dependencies list.

I'm not sure how feasible such refactoring would be but it's certainly not a question of changing two lines.

@staabm

staabm Jun 1, 2015

Contributor

@mzgol but src/ajax returns jQuery (which gets exported then) in https://github.com/jquery/jquery/blob/master/src/ajax.js#L808 , so it should be available because ajax ===jQuery ?

@mgol

mgol Jun 1, 2015

Member

This would be confusing, ajax should mean jQuery.ajax. Also, if you go by this route you cannot have a guarantee you choose the proper name and not miss a dependency or not declare a fake dependency that shouldn't be there. So I don't think we should do it this way, I don't really see the gain.

+], function( jQuery, support ) {
jQuery.ajaxSettings.xhr = function() {
try {
@@ -33,13 +33,13 @@ if ( window.ActiveXObject ) {
});
}
-jQuery.support.cors = !!xhrSupported && ( "withCredentials" in xhrSupported );
-jQuery.support.ajax = xhrSupported = !!xhrSupported;
+support.cors = !!xhrSupported && ( "withCredentials" in xhrSupported );
+support.ajax = xhrSupported = !!xhrSupported;
jQuery.ajaxTransport(function( options ) {
var callback;
// Cross domain only allowed if supported through XMLHttpRequest
- if ( jQuery.support.cors || xhrSupported && !options.crossDomain ) {
+ if ( support.cors || xhrSupported && !options.crossDomain ) {
return {
send: function( headers, complete ) {
var i, id,
View
@@ -2,9 +2,9 @@ define([
"../core",
"../var/rnotwhite",
"../var/strundefined",
- "../selector",
- "../support"
-], function( jQuery, rnotwhite, strundefined ) {
+ "./support",
+ "../selector"
+], function( jQuery, rnotwhite, strundefined, support ) {
var nodeHook, boolHook,
attrHandle = jQuery.expr.attrHandle;
@@ -93,7 +93,8 @@ jQuery.extend({
attrHooks: {
type: {
set: function( elem, value ) {
- if ( !jQuery.support.radioValue && value === "radio" && jQuery.nodeName(elem, "input") ) {
+ if ( !support.radioValue && value === "radio" &&
+ jQuery.nodeName( elem, "input" ) ) {
// Setting the type on a radio button after the value resets the value in IE6-9
// Reset value to default in case type is set after value during creation
var val = elem.value;
View
@@ -1,7 +1,7 @@
define([
"../core",
- "../support"
-], function( jQuery ) {
+ "./support"
+], function( jQuery, support ) {
var rfocusable = /^(?:input|select|textarea|button)$/i;
@@ -65,7 +65,7 @@ jQuery.extend({
// Support: IE9+
// Selectedness for an option in an optgroup can be inaccurate
-if ( !jQuery.support.optSelected ) {
+if ( !support.optSelected ) {
jQuery.propHooks.selected = {
get: function( elem ) {
var parent = elem.parentNode;
View
@@ -0,0 +1,35 @@
+define([
+ "../var/support"
+], function( support ) {
+
+(function () {
+ var input = document.createElement( "input" ),
+ select = document.createElement( "select" ),
+ opt = select.appendChild( document.createElement( "option" ) );
+
+ input.type = "checkbox";
+
+ // Support: Safari 5.1, iOS 5.1, Android 4.x, Android 2.3
+ // Check the default checkbox/radio value ("" on old WebKit; "on" elsewhere)
+ support.checkOn = input.value !== "";
+
+ // Must access the parent to make an option select properly
+ // Support: IE9, IE10
+ support.optSelected = opt.selected;
+
+ // Make sure that the options inside disabled selects aren't marked as disabled
+ // (WebKit marks them as disabled)
+ select.disabled = true;
+ support.optDisabled = !opt.disabled;
+
+ // Check if an input maintains its value after becoming a radio
+ // Support: IE9, IE10
+ input = document.createElement( "input" );
+ input.value = "t";
+ input.type = "radio";
+ support.radioValue = input.value === "t";
+})();
+
+return support;
+
+});
View
@@ -1,7 +1,7 @@
define([
"../core",
- "../support"
-], function( jQuery ) {
+ "./support"
+], function( jQuery, support ) {
var rreturn = /\r/g;
@@ -95,7 +95,7 @@ jQuery.extend({
// IE6-9 doesn't update selected after form reset (#2551)
if ( ( option.selected || i === index ) &&
// Don't return options that are disabled or in a disabled optgroup
- ( jQuery.support.optDisabled ? !option.disabled : option.getAttribute("disabled") === null ) &&
+ ( support.optDisabled ? !option.disabled : option.getAttribute( "disabled" ) === null ) &&
( !option.parentNode.disabled || !jQuery.nodeName( option.parentNode, "optgroup" ) ) ) {
// Get the specific value for the option
@@ -146,7 +146,7 @@ jQuery.each([ "radio", "checkbox" ], function() {
}
}
};
- if ( !jQuery.support.checkOn ) {
+ if ( support.checkOn ) {
jQuery.valHooks[ this ].get = function( elem ) {
// Support: Webkit
// "" is returned instead of "on" if a value isn't specified
View
@@ -9,9 +9,10 @@ define([
"./var/class2type",
"./var/toString",
"./var/hasOwn",
- "./var/trim"
+ "./var/trim",
+ "./var/support"
], function( strundefined, arr, slice, concat, push, indexOf,
- class2type, toString, hasOwn, trim ) {
+ class2type, toString, hasOwn, trim, support ) {
var
// A central reference to the root jQuery(document)
@@ -702,7 +703,11 @@ jQuery.extend({
length ? fn( elems[0], key ) : emptyGet;
},
- now: Date.now
+ now: Date.now,
+
+ // jQuery.support is not used in Core but other projects attach their
+ // properties to it so it needs to exist.
+ support: support
});
// Populate the class2type map
View
@@ -3,15 +3,15 @@ define([
"./var/pnum",
"./css/var/cssExpand",
"./css/var/isHidden",
+ "./css/support",
"./css/defaultDisplay",
"./data/var/data_priv",
- "./core/swap",
+ "./css/swap",
"./core/ready",
"./selector", // contains
- "./support",
// Optional
"./offset"
-], function( jQuery, pnum, cssExpand, isHidden, defaultDisplay, data_priv ) {
+], function( jQuery, pnum, cssExpand, isHidden, support, defaultDisplay, data_priv ) {
var curCSS,
// swappable if display is none or starts with table except "table", "table-cell", or "table-caption"
// see here for display values: https://developer.mozilla.org/en-US/docs/CSS/display
@@ -233,7 +233,7 @@ jQuery.extend({
// Fixes #8908, it can be done more correctly by specifying setters in cssHooks,
// but it would mean to define eight (for every problematic property) identical functions
- if ( !jQuery.support.clearCloneStyle && value === "" && name.indexOf("background") === 0 ) {
+ if ( !support.clearCloneStyle && value === "" && name.indexOf( "background" ) === 0 ) {
style[ name ] = "inherit";
}
@@ -382,7 +382,7 @@ function getWidthOrHeight( elem, name, extra ) {
var valueIsBorderBox = true,
val = name === "width" ? elem.offsetWidth : elem.offsetHeight,
styles = getStyles( elem ),
- isBorderBox = jQuery.support.boxSizing && jQuery.css( elem, "boxSizing", false, styles ) === "border-box";
+ isBorderBox = jQuery.css( elem, "boxSizing", false, styles ) === "border-box";
// some non-html elements return undefined for offsetWidth, so check for null/undefined
// svg - https://bugzilla.mozilla.org/show_bug.cgi?id=649285
@@ -401,7 +401,8 @@ function getWidthOrHeight( elem, name, extra ) {
// we need the check for style in case a browser which returns unreliable values
// for getComputedStyle silently falls back to the reliable elem.style
- valueIsBorderBox = isBorderBox && ( jQuery.support.boxSizingReliable || val === elem.style[ name ] );
+ valueIsBorderBox = isBorderBox &&
+ ( support.boxSizingReliable() || val === elem.style[ name ] );
// Normalize "", auto, and prepare for extra
val = parseFloat( val ) || 0;
@@ -440,50 +441,57 @@ jQuery.each([ "height", "width" ], function( i, name ) {
elem,
name,
extra,
- jQuery.support.boxSizing && jQuery.css( elem, "boxSizing", false, styles ) === "border-box",
+ jQuery.css( elem, "boxSizing", false, styles ) === "border-box",
styles
) : 0
);
}
};
});
-// These hooks cannot be added until DOM ready because the support test
-// for it is not run until after DOM ready
-jQuery(function() {
- // Support: Android 2.3
- if ( !jQuery.support.reliableMarginRight ) {
- jQuery.cssHooks.marginRight = {
- get: function( elem, computed ) {
- if ( computed ) {
- // Support: Android 2.3
- // WebKit Bug 13343 - getComputedStyle returns wrong value for margin-right
- // Work around by temporarily setting element display to inline-block
- return jQuery.swap( elem, { "display": "inline-block" },
- curCSS, [ elem, "marginRight" ] );
- }
- }
- };
+// Support: Android 2.3
+jQuery.cssHooks.marginRight = {
+ get: function( elem, computed ) {
+ if ( support.reliableMarginRight() ) {
+ // Hook not needed, remove it.
+ // Since there are no other hooks for marginRight, remove the whole object.
+ delete jQuery.cssHooks.marginRight;
+ return;
+ }
+ if ( computed ) {
+ // Support: Android 2.3
+ // WebKit Bug 13343 - getComputedStyle returns wrong value for margin-right
+ // Work around by temporarily setting element display to inline-block
+ return jQuery.swap( elem, { "display": "inline-block" },
+ curCSS, [ elem, "marginRight" ] );
+ }
}
+};
- // Webkit bug: https://bugs.webkit.org/show_bug.cgi?id=29084
- // getComputedStyle returns percent when specified for top/left/bottom/right
- // rather than make the css module depend on the offset module, we just check for it here
- if ( !jQuery.support.pixelPosition && jQuery.fn.position ) {
- jQuery.each( [ "top", "left" ], function( i, prop ) {
- jQuery.cssHooks[ prop ] = {
- get: function( elem, computed ) {
- if ( computed ) {
- computed = curCSS( elem, prop );
- // if curCSS returns percentage, fallback to offset
- return rnumnonpx.test( computed ) ?
- jQuery( elem ).position()[ prop ] + "px" :
- computed;
- }
+// Webkit bug: https://bugs.webkit.org/show_bug.cgi?id=29084
+// getComputedStyle returns percent when specified for top/left/bottom/right
+// rather than make the css module depend on the offset module, we just check for it here
+jQuery.each( [ "top", "left" ], function( i, prop ) {
+ jQuery.cssHooks[ prop ] = {
+ get: function( elem, computed ) {
+ if ( support.pixelPosition() || !jQuery.fn.position ) {
+ // Hook not needed, remove it.
+ // Since there are no other hooks for prop, remove the whole object.
+ delete jQuery.cssHooks[ prop ];
+ return;
+ }
+ jQuery.cssHooks[ prop ].get = function ( i, prop ) {
+ if ( computed ) {
+ computed = curCSS( elem, prop );
+ // if curCSS returns percentage, fallback to offset
+ return rnumnonpx.test( computed ) ?
+ jQuery( elem ).position()[ prop ] + "px" :
+ computed;
}
};
- });
- }
+ return jQuery.cssHooks[ prop ].get( i, prop );
+ }
+ };
});
// These hooks are used by animate to expand properties
Oops, something went wrong.

10 comments on commit bbbdd94

Member

jdalton commented on bbbdd94 Jan 24, 2014

I've missed where the tests are lazy (can't seem to find it), they look like they are all initialized once the module is loaded.

Owner

timmywil replied Jan 24, 2014

This commit split up tests into their respective modules, but not all tests need to be lazy. The css module has examples of tests run lazily. Also, there are more in 1.x than 2.x.

Member

jdalton replied Jan 24, 2014

@timmywil Thanks! In looking at the css module for 2+ it looks like the lazy call reliableMarginRight is not caching its return value in reliableMarginRightVal like others (it's v1 counterpart is though).

Owner

timmywil replied Jan 24, 2014

@jdalton Some of the support tests work in conjunction with addGetHookIf. This function attaches a get hook that calls the support function on its first call and then replaces itself with the appropriate result. Effectively, reliableMarginRight is only called once.

Member

jdalton replied Jan 24, 2014

Rad. So in that case the v1 counterpart doesn't need to cache its result (it's currently caching it) in reliableMarginRightVal either?

Owner

timmywil replied Jan 24, 2014

@jdalton agreed.

@mzgol Also, looking at the 1.x code. It seems setting some support values ahead of time, when computedStyleTests() gets run first, ensures that the support tests never actually get run.

Member

mgol replied Jan 25, 2014

@jdalton Nice catch with reliableMarginRight in 1.x! Thanks, I've missed it. The strategy of caching only those tests that are invoked more than once could backfire if the support test set changed frequently but it's pretty stable so I went for it.

@timmywil It's actually only a question of reliableMarginRight as the rest is set just a few lines later:
https://github.com/jquery/jquery/blob/41523ae1d35b9023e9479b58c0fbe47269746411/src/css/support.js#L178-179
I guess we don't have a test that would force-compute boxSizingReliable and after that reliableMarginRight, otherwise it'd be caught. Not sure if it's worth adding a test for that, though - the number of potential combinations of order in which support tests are executed is quite high and once I remove the unneeded caching of this support test result, the problem will solve itself automatically.

Member

mgol replied Jan 25, 2014

@timmywil BTW, why do we need to test boxSizing via checking offsetWidth, was some browser misbehaving? Looking at support tables: https://github.com/jquery/jquery/blob/master/test/unit/support.js it seems every browser that supports the box-sizing property has true there. In that case, it would be simpler to just check if the boxSizing rule doesn't disappear. It would also allow to remove the jQuery.swap workaround and would make it possible to run this test without attaching the div to body.

Owner

timmywil replied Jan 25, 2014

once I remove the unneeded caching of this support test result, the problem will solve itself automatically.

👍

Please sign in to comment.