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

Offset Module: Get Rid of getWindow Function #3449

Closed
david-mark opened this Issue Dec 12, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@david-mark

david-mark commented Dec 12, 2016

Description

Working on deprecating isWindow, I ran into this internal function in the offset module:

/**
 * Gets a window from an element
 */
function getWindow( elem ) {
	return jQuery.isWindow( elem ) ? elem : elem.nodeType === 9 && elem.defaultView;
}

The comment is incorrect. It returns a reference to window if passed a reference to a window or document node, but it returns false if passed a reference to an element node.

As a side note, it seems very odd to try to derive a window reference at all, considering that many methods simply reference the window that is passed to the core during initialization.

For example:

// Miscellaneous
"target": function( elem ) {
	var hash = window.location && window.location.hash;
	return hash && hash.slice( 1 ) === elem.id;
}

Or perhaps that's just a bug. Do see that some methods at least attempt to work with multiple windows. But I digress.

The inappropriately named getWindow function is used in two places. As always, if we can reduce that number to one, we can get rid of the function (and the costs associated with calling it).

First one is in the offset method:

doc = elem.ownerDocument;
win = getWindow( doc );

So that obviously reduces to:

doc = elem.ownerDocument;
win = doc.defaultView;

The other is here:

// Create scrollLeft and scrollTop methods
jQuery.each( { scrollLeft: "pageXOffset", scrollTop: "pageYOffset" }, function( method, prop ) {
	var top = "pageYOffset" === prop;

	jQuery.fn[ method ] = function( val ) {
		return access( this, function( elem, method, val ) {
			var win = getWindow( elem );

			if ( val === undefined ) {
				return win ? win[ prop ] : elem[ method ];
			}

			if ( win ) {
				win.scrollTo(
					!top ? val : win.pageXOffset,
					top ? val : win.pageYOffset
				);

			} else {
				elem[ method ] = val;
			}
		}, method, val, arguments.length );
	};
} );

That confirms the function is expected to return a "falsey" value when passed an element reference. Something like this should replace the getWindow call:

var win;
			
if ( elem.window === elem ) {
	win = elem;
} else if ( elem.nodeType == 9 ) {
	win = elem.defaultView;
}

...and then that function can be deleted.

One less isWindow call in the bargain as well. As that one is public, it will have to be deprecated rather than deleted. That's next.

HTH

PS. Consider that a large number of issues are related to the ill-advised design of the object model, which tries to deal with elements, documents and windows (among other things) using a single constructor. It's an awkward interface for users as well. Was a similar mistake with attr, which tried to deal with both (HTML) properties and (XML) attributes using a single function.

Link to test case

No test case needed. Simply restructuring.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Dec 12, 2016

Member

I agree with the proposed changes to eliminate getWindow. Pull request welcome.

Member

gibson042 commented Dec 12, 2016

I agree with the proposed changes to eliminate getWindow. Pull request welcome.

@markelog markelog added the Core label Dec 12, 2016

@david-mark

This comment has been minimized.

Show comment
Hide comment
@david-mark

david-mark Dec 16, 2016

Not sure how to do that with my fork in its current state. Would have to create another fork I expect. This change came about while eliminating the bigger issue of isWindow and there are a dozen other changes in my fork at this point.

Might be easier for somebody else to fix this based on my suggestions. As was the case with this one (which was closed due prematurely for same reason):

#3444

david-mark commented Dec 16, 2016

Not sure how to do that with my fork in its current state. Would have to create another fork I expect. This change came about while eliminating the bigger issue of isWindow and there are a dozen other changes in my fork at this point.

Might be easier for somebody else to fix this based on my suggestions. As was the case with this one (which was closed due prematurely for same reason):

#3444

gibson042 added a commit to gibson042/jquery that referenced this issue Dec 16, 2016

@gibson042 gibson042 added this to the 3.2.0 milestone Dec 19, 2016

@jquery jquery deleted a comment Nov 9, 2017

@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018

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