fixes #10996 (simplify offset) #642

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
8 participants
@mikesherov
Member

mikesherov commented Dec 25, 2011

In all of jQuery's supported browsers, either getBoundingClientRect() or window.webkitConvertPointFromNodeToPage() exists, thereby eliminating the need for the old lengthy getOffset function. Good riddance!

http://bugs.jquery.com/ticket/10996

In case anyone is curious, and I know you are, here is the size diff:

  247053  (-2576) jquery.js
   92588  (-1462) jquery.min.js
   32937   (-448) jquery.min.js.gz
@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Dec 25, 2011

Member

Just one last thing, all 3 of my open pull requests will probably conflict with each other:

#639
#630

Member

mikesherov commented Dec 25, 2011

Just one last thing, all 3 of my open pull requests will probably conflict with each other:

#639
#630

@jaubourg

View changes

src/offset.js
try {
box = elem.getBoundingClientRect();
} catch(e) {}
// Make sure we're not dealing with a disconnected DOM node
if ( !box || !jQuery.contains( docElem, elem ) ) {
- return box ? { top: box.top, left: box.left } : { top: 0, left: 0 };
+ return box ? { top: box.top, left: box.left } : noOffset;

This comment has been minimized.

@jaubourg

jaubourg Dec 25, 2011

Member

I wouldn't do that. I know I happen to use the object returned here and modify it... better return a new object all the time.

@jaubourg

jaubourg Dec 25, 2011

Member

I wouldn't do that. I know I happen to use the object returned here and modify it... better return a new object all the time.

This comment has been minimized.

@jaubourg

jaubourg Dec 25, 2011

Member

Oh and merry christmas! :)

@jaubourg

jaubourg Dec 25, 2011

Member

Oh and merry christmas! :)

This comment has been minimized.

@mikesherov

mikesherov Dec 25, 2011

Member

O.o

I completely forgot that the object would be by reference! Making the change shortly. And merry Christmas to you as well.

@mikesherov

mikesherov Dec 25, 2011

Member

O.o

I completely forgot that the object would be by reference! Making the change shortly. And merry Christmas to you as well.

This comment has been minimized.

@gibson042

gibson042 Jan 4, 2012

Member

First: awesome job; bon voyage to manual getOffset!

Second: You can save another 8/10/2 bytes here with { top: box ? box.top : 0, left: box ? box.left : 0 }... or 19/14/2 with jQuery.extend( { top: 0, left: 0 }, box ) (but that would come with extraneous right and bottom properties).

@gibson042

gibson042 Jan 4, 2012

Member

First: awesome job; bon voyage to manual getOffset!

Second: You can save another 8/10/2 bytes here with { top: box ? box.top : 0, left: box ? box.left : 0 }... or 19/14/2 with jQuery.extend( { top: 0, left: 0 }, box ) (but that would come with extraneous right and bottom properties).

This comment has been minimized.

@mikesherov

mikesherov Jan 4, 2012

Member

I'm holding off on further changes until I can get some confirmation for jQuery Mobile team that this is safe.

@mikesherov

mikesherov Jan 4, 2012

Member

I'm holding off on further changes until I can get some confirmation for jQuery Mobile team that this is safe.

@laughinghan

This comment has been minimized.

Show comment
Hide comment
@laughinghan

laughinghan Mar 28, 2012

is it just me or is this broken?

$ make
Building ./dist/jquery.js
Minifying jQuery ./dist/jquery.min.js
Checking jQuery against JSHint...
JSHint found errors.
 [L1562:C52] 'paddingMarginBorder' is not defined.
  div.innerHTML = "<table><tr><td style='" + paddingMarginBorder + "0;display:none'></td><td>t</td></tr></table>";

 [L1580:C13] 'marginDiv' is not defined.
  marginDiv = document.createElement( "div" );

 [L1581:C13] 'marginDiv' is not defined.
  marginDiv.style.width = "0";

 [L1582:C13] 'marginDiv' is not defined.
  marginDiv.style.marginRight = "0";

 [L1584:C30] 'marginDiv' is not defined.
  div.appendChild( marginDiv );

 [L1586:C56] 'marginDiv' is not defined.
  ( parseInt( ( window.getComputedStyle( marginDiv, null ) || { marginRight: 0 } ).marginRight, 10 ) || 0 ) === 0;

 [L1630:C9] 'marginDiv' is not defined.
  marginDiv = div = container = null;

jQuery Size - compared to last make
  250149  (-2738) jquery.js
   93502  (-1344) jquery.min.js
   33258   (-391) jquery.min.js.gz
jQuery build complete.

is it just me or is this broken?

$ make
Building ./dist/jquery.js
Minifying jQuery ./dist/jquery.min.js
Checking jQuery against JSHint...
JSHint found errors.
 [L1562:C52] 'paddingMarginBorder' is not defined.
  div.innerHTML = "<table><tr><td style='" + paddingMarginBorder + "0;display:none'></td><td>t</td></tr></table>";

 [L1580:C13] 'marginDiv' is not defined.
  marginDiv = document.createElement( "div" );

 [L1581:C13] 'marginDiv' is not defined.
  marginDiv.style.width = "0";

 [L1582:C13] 'marginDiv' is not defined.
  marginDiv.style.marginRight = "0";

 [L1584:C30] 'marginDiv' is not defined.
  div.appendChild( marginDiv );

 [L1586:C56] 'marginDiv' is not defined.
  ( parseInt( ( window.getComputedStyle( marginDiv, null ) || { marginRight: 0 } ).marginRight, 10 ) || 0 ) === 0;

 [L1630:C9] 'marginDiv' is not defined.
  marginDiv = div = container = null;

jQuery Size - compared to last make
  250149  (-2738) jquery.js
   93502  (-1344) jquery.min.js
   33258   (-391) jquery.min.js.gz
jQuery build complete.

jaubourg and others added some commits Apr 1, 2012

Makes Deferred implementation truly Promise/A compliant. Unit tests a…
…mended. Actually few changes required in jQuery's own source and we gained 8 bytes minified gzipped \o/.
$.ajax now always returns an object implementing the Promise interfac…
…e. Fixes #10944. Unit tests amended.

For back-compat, in case of an early abort, callbacks passed in the options are not called (while subsequent callbacks attached to the returned Promise are).
For early abort triggered by returning false in beforeSend, statusText is "canceled".
For much improved consistency, jqXHR.abort() sets a default statusTex…
…t of 'canceled' right until after beforeSend has been called (in which case it reverts to the default of 'abort'): now all early aborts have a statusText of 'canceled'.
function parity with getBoundingClientRect
return new object literal in all cases
style and JSHint fixes

@mikesherov mikesherov closed this Apr 3, 2012

@briancavalier

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Jun 12, 2012

Cool, it's great to see this headed toward Promises/A! I think it's gonna become the norm for multiple promise implementations to be in play in any reasonably interesting app. I found a situation where I believe it still isn't fully compliant, so I created a quick test case. It should show 3 for both jQuery and when.js, but the jQuery.Deferred seems never to resolve.

Cool, it's great to see this headed toward Promises/A! I think it's gonna become the norm for multiple promise implementations to be in play in any reasonably interesting app. I found a situation where I believe it still isn't fully compliant, so I created a quick test case. It should show 3 for both jQuery and when.js, but the jQuery.Deferred seems never to resolve.

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Jun 13, 2012

Member

It seems to me that when.js wrongly calls the last resolve callback. I'd expect this to be the correct code and behaviour.

Member

jaubourg replied Jun 13, 2012

It seems to me that when.js wrongly calls the last resolve callback. I'd expect this to be the correct code and behaviour.

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Jun 13, 2012

Hey @jaubourg,

I'm pretty sure the correct behavior is that the promise chain should return to resolving when a registered rejection handler does not explicitly propagate the rejection by either throwing or returning a rejected promise--i.e. it should behave similar to a catch statement. To propagate the exception, a catch statement must explicitly rethrow (or throw a new exception). If it doesn't explicitly (re)throw, the original exception is considered to be handled and doesn't propagate.

I've updated the original fiddle to include Q, which matches when.js's behavior. I also created a new fiddle that includes when.js, Q, and dojo (1.6, because it's easier to use in jsfiddle than 1.7, but the behavior is the same). Those show the same behavior, so I think jQuery is the outlier in this case.

Please don't take this the wrong way. Like I said, I'm really interested in trying to ensure that the most popular promise implementations are all interoperable, and hopefully this is a test case that will help.

Hey @jaubourg,

I'm pretty sure the correct behavior is that the promise chain should return to resolving when a registered rejection handler does not explicitly propagate the rejection by either throwing or returning a rejected promise--i.e. it should behave similar to a catch statement. To propagate the exception, a catch statement must explicitly rethrow (or throw a new exception). If it doesn't explicitly (re)throw, the original exception is considered to be handled and doesn't propagate.

I've updated the original fiddle to include Q, which matches when.js's behavior. I also created a new fiddle that includes when.js, Q, and dojo (1.6, because it's easier to use in jsfiddle than 1.7, but the behavior is the same). Those show the same behavior, so I think jQuery is the outlier in this case.

Please don't take this the wrong way. Like I said, I'm really interested in trying to ensure that the most popular promise implementations are all interoperable, and hopefully this is a test case that will help.

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Jun 13, 2012

Member

Well, Brian, it's probably the first time I see jQuery being called the "outlier" rather than the nasty-hype-machine-that-will-devour-the-world (tm), so it's quite refreshing ;)

This use-case is probably yet another nail in the coffin of jQuery's Promise/A compliance. You probably followed the multiple discussions that took place about this. Equating exceptions and rejections is something we cannot do in a our environment: it makes debugging impossible. The metaphor is broken, btw, because, with Promise/A, exceptions are always handled (which is the gist of the problem). But let's not beat a dead horse.

Here is why I have a problem with rejection handlers changing state:

// Promise/A
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => resolved
promise.then( null, null, function() {}  ); // => pending

// jQuery
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => rejected
promise.then( null, null, function() {}  ); // => pending

"Redirecting" state has to be done explicitly in jQuery's implementation: it's explicit and it's symmetrical.

If people want us to write down a spec of jQuery's take on Promises, I'll try and find time to do so (though I'd love to get some help on this one). However, pushing jQuery's implementation forward to be fully Promise/A compliant is probably never gonna happen at this point. As minimal as Promise/A appeared to be, it has one contraint too many, at least for us.

Member

jaubourg replied Jun 13, 2012

Well, Brian, it's probably the first time I see jQuery being called the "outlier" rather than the nasty-hype-machine-that-will-devour-the-world (tm), so it's quite refreshing ;)

This use-case is probably yet another nail in the coffin of jQuery's Promise/A compliance. You probably followed the multiple discussions that took place about this. Equating exceptions and rejections is something we cannot do in a our environment: it makes debugging impossible. The metaphor is broken, btw, because, with Promise/A, exceptions are always handled (which is the gist of the problem). But let's not beat a dead horse.

Here is why I have a problem with rejection handlers changing state:

// Promise/A
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => resolved
promise.then( null, null, function() {}  ); // => pending

// jQuery
promise.then( function() {}  ); // => resolved
promise.then( null, function() {}  ); // => rejected
promise.then( null, null, function() {}  ); // => pending

"Redirecting" state has to be done explicitly in jQuery's implementation: it's explicit and it's symmetrical.

If people want us to write down a spec of jQuery's take on Promises, I'll try and find time to do so (though I'd love to get some help on this one). However, pushing jQuery's implementation forward to be fully Promise/A compliant is probably never gonna happen at this point. As minimal as Promise/A appeared to be, it has one contraint too many, at least for us.

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Jun 14, 2012

I'm def not here to be a jQuery-hater, just trying to ensure some interoperability.

I disagree with your feelings about Promises/A, and I don't see Promises/A as having too many constraints at all. In fact, I see it as quite simple:

  1. If a handler returns successfully, the next promise is resolved.
  2. If a handler throws or returns a rejection, the next promise is rejected.

That said, I respect that every project has its own goals and constraints. If it doesn't make sense for jQuery to provide a Promises/A compliant promise, then I do think it's important to document its promise behavior.

Given the current (good) trend of more modular code, and moving more responsiblity (and therefore, code) to the browser-side, it's inevitable that applications will contain multiple promise implementations. Having an equalizer like when.js's when(), that can consume all of the above will help.

I think it would make sense for jQuery documentation to point people to when.js or Q as ways to enable promise interoperability.

I'm def not here to be a jQuery-hater, just trying to ensure some interoperability.

I disagree with your feelings about Promises/A, and I don't see Promises/A as having too many constraints at all. In fact, I see it as quite simple:

  1. If a handler returns successfully, the next promise is resolved.
  2. If a handler throws or returns a rejection, the next promise is rejected.

That said, I respect that every project has its own goals and constraints. If it doesn't make sense for jQuery to provide a Promises/A compliant promise, then I do think it's important to document its promise behavior.

Given the current (good) trend of more modular code, and moving more responsiblity (and therefore, code) to the browser-side, it's inevitable that applications will contain multiple promise implementations. Having an equalizer like when.js's when(), that can consume all of the above will help.

I think it would make sense for jQuery documentation to point people to when.js or Q as ways to enable promise interoperability.

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Jun 15, 2012

I'll add that from my perspective, @dmethvin's question is one of application design, and not of promises or exceptions. The data used as an error payload is, imho, separate from the mechanism that transports the error. Rejecting a promise with custom data is analogous to throwing that custom data. Same data, different transport. If your application design is one that uses custom data in that way, then, as @domenic described, Promises/A simply gives you an asynchronous transport that is analogous to the synchronous transport provided by throw. Any application-level logic built around the data would remain nearly identical when using Promises/A, as Domenic's gist shows.

I'll add that from my perspective, @dmethvin's question is one of application design, and not of promises or exceptions. The data used as an error payload is, imho, separate from the mechanism that transports the error. Rejecting a promise with custom data is analogous to throwing that custom data. Same data, different transport. If your application design is one that uses custom data in that way, then, as @domenic described, Promises/A simply gives you an asynchronous transport that is analogous to the synchronous transport provided by throw. Any application-level logic built around the data would remain nearly identical when using Promises/A, as Domenic's gist shows.

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Jun 15, 2012

@scottgonzalez it depends entirely on what was thrown. You can throw anything, and a catch statement higher up will receive it verbatim:

node -e 'try { throw "hello world"; } catch(message) { console.log(message, typeof message); }'

I'm certainly not advocating that in application code ;)

@scottgonzalez it depends entirely on what was thrown. You can throw anything, and a catch statement higher up will receive it verbatim:

node -e 'try { throw "hello world"; } catch(message) { console.log(message, typeof message); }'

I'm certainly not advocating that in application code ;)

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 15, 2012

Member

Let me try to make this more concrete, so I can tell whether this is a misunderstanding on my part or yours of the impact this would have to existing jQuery code.

jQuery wears several hats in this conversation: 1) An implementer of something that aspires to be Promise/A compliant. 2) A client of the current $.Deferred implementation, for example in $.ajax. and $.fn.animate. 3) A provider of the current $.Deferred implementation to jQuery developers.

Let's say someone has included jQuery in their web page and writes this code:

$.ajax(url).then(
    function(jqXHR) { /* do successful stuff */ },
    function(jqXHR, errorCode) { /* deal with failed request */ }
);

If some code or callback has a code error today, the browser throws an uncaught error with all sorts of detail which is visible on the console. It can also be caught by a window.onerror handler and sent back to a server for analysis using tools such as DamnIT, Errorception, AirBrake, Runtime Intelligence, or Google Analytics. For unanticipated script errors that fall into the "Never check for an error condition you don't know how to handle" category, this is by far the lowest-hassle way to go.

The proposal on the table as I understand it is to catch the error and pass it to the fail handler, which is expecting the (jqXHR, errorCode) it always got in the past but now has to deal with (Error) as well. The same goes for a developer's direct use of jQuery $.Deferred. So to handle a case that is supposedly a "de-facto standard" (of the Promise/A proposal which is not a standard) we should put on Steve's hat and tell everyone to go back and rewrite their code to accommodate this? Plus, it's again asking the developer to handle a totally unanticipated condition, and giving them less information than if they had let it get to the window.onerror handler or the browser's error console.

I have been talking with @jaubourg about how we might be able to change the semantics with a flag to make it do this, but even so this seems like it would have to be opt-in or we'd be breaking code all over the place. And to prevent further hassles, we should be recommending that all fail handlers take a single Error or Error-like argument unless the function is willing to sniff around (which seems incredibly ugly to me). Still, as large as the jQuery ecosystem is, there would be a mix of old-behavior and new-behavior code out there for years to come.

Is it clear now why we can't just change the fundamental behavior of our promise implementation? Or is there some easy way out? If the issue is simply that we're not really Promise/A and can't become so without breaking user code, one option would simply be to publicize that our promises implementation isn't interoperable and explain why we placed user code compatibility over interoperability.

Member

dmethvin replied Jun 15, 2012

Let me try to make this more concrete, so I can tell whether this is a misunderstanding on my part or yours of the impact this would have to existing jQuery code.

jQuery wears several hats in this conversation: 1) An implementer of something that aspires to be Promise/A compliant. 2) A client of the current $.Deferred implementation, for example in $.ajax. and $.fn.animate. 3) A provider of the current $.Deferred implementation to jQuery developers.

Let's say someone has included jQuery in their web page and writes this code:

$.ajax(url).then(
    function(jqXHR) { /* do successful stuff */ },
    function(jqXHR, errorCode) { /* deal with failed request */ }
);

If some code or callback has a code error today, the browser throws an uncaught error with all sorts of detail which is visible on the console. It can also be caught by a window.onerror handler and sent back to a server for analysis using tools such as DamnIT, Errorception, AirBrake, Runtime Intelligence, or Google Analytics. For unanticipated script errors that fall into the "Never check for an error condition you don't know how to handle" category, this is by far the lowest-hassle way to go.

The proposal on the table as I understand it is to catch the error and pass it to the fail handler, which is expecting the (jqXHR, errorCode) it always got in the past but now has to deal with (Error) as well. The same goes for a developer's direct use of jQuery $.Deferred. So to handle a case that is supposedly a "de-facto standard" (of the Promise/A proposal which is not a standard) we should put on Steve's hat and tell everyone to go back and rewrite their code to accommodate this? Plus, it's again asking the developer to handle a totally unanticipated condition, and giving them less information than if they had let it get to the window.onerror handler or the browser's error console.

I have been talking with @jaubourg about how we might be able to change the semantics with a flag to make it do this, but even so this seems like it would have to be opt-in or we'd be breaking code all over the place. And to prevent further hassles, we should be recommending that all fail handlers take a single Error or Error-like argument unless the function is willing to sniff around (which seems incredibly ugly to me). Still, as large as the jQuery ecosystem is, there would be a mix of old-behavior and new-behavior code out there for years to come.

Is it clear now why we can't just change the fundamental behavior of our promise implementation? Or is there some easy way out? If the issue is simply that we're not really Promise/A and can't become so without breaking user code, one option would simply be to publicize that our promises implementation isn't interoperable and explain why we placed user code compatibility over interoperability.

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Jun 15, 2012

Ok, I think it's time step back for a moment and remember how this thread started by rereading the commit message and my initial comment.

The stated intent of the commit was for jQuery Deferred to be made Promises/A compliant. I came upon it, and was very happy to see this. In an effort to try to help, I supplied a test case where post-this-commit, it isn't compliant. I think it's also important to note that both @domenic and I have said that we respect that jQuery has it's own goals and priorities, like every project. Neither of us has in any way demanded that jQuery Deferred become Promises/A. The intent to do so was implied by the commit.

If Promises/A is not a desirable goal of jQuery Deferred, I respect that. Unfortunately, I believe there is pain for developers either way.

My original suggestion was for jQuery documentation to point developers who need or desire promise interoperability to when.js or Q, as their when() functions can provide it.

And @jaubourg suggested documenting jQuery's promise behavior.

I stand by both of those as reasonable suggestions that would benefit jQuery, implementors of other libraries, and the larger Javascript developer community.

Ok, I think it's time step back for a moment and remember how this thread started by rereading the commit message and my initial comment.

The stated intent of the commit was for jQuery Deferred to be made Promises/A compliant. I came upon it, and was very happy to see this. In an effort to try to help, I supplied a test case where post-this-commit, it isn't compliant. I think it's also important to note that both @domenic and I have said that we respect that jQuery has it's own goals and priorities, like every project. Neither of us has in any way demanded that jQuery Deferred become Promises/A. The intent to do so was implied by the commit.

If Promises/A is not a desirable goal of jQuery Deferred, I respect that. Unfortunately, I believe there is pain for developers either way.

My original suggestion was for jQuery documentation to point developers who need or desire promise interoperability to when.js or Q, as their when() functions can provide it.

And @jaubourg suggested documenting jQuery's promise behavior.

I stand by both of those as reasonable suggestions that would benefit jQuery, implementors of other libraries, and the larger Javascript developer community.

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 15, 2012

@briancavalier basically said everything I would want to say. I just jumped in to help educate and clarify, not to recommend Promises/A interoperation---I assumed that battle was already lost. As you say, @dmethvin, there's really no easy way out.

The only thing I would argue for, that nobody else seems to be behind, is dropping then completely so that jQuery properly fails the Promises/A duck-type test. This makes a lot of sense to me: as of 1.7, it's just a less powerful pipe, and most examples use and encourage done anyway. So perhaps deprecate in 1.8, remove in 1.9. But it's not that hard to deal with anyway, so whatevs.

@briancavalier basically said everything I would want to say. I just jumped in to help educate and clarify, not to recommend Promises/A interoperation---I assumed that battle was already lost. As you say, @dmethvin, there's really no easy way out.

The only thing I would argue for, that nobody else seems to be behind, is dropping then completely so that jQuery properly fails the Promises/A duck-type test. This makes a lot of sense to me: as of 1.7, it's just a less powerful pipe, and most examples use and encourage done anyway. So perhaps deprecate in 1.8, remove in 1.9. But it's not that hard to deal with anyway, so whatevs.

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