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

Changes to offset break jQuery UI #2310

Closed
arschmitz opened this Issue May 13, 2015 · 43 comments

Comments

Projects
None yet
8 participants
@arschmitz
Member

arschmitz commented May 13, 2015

1617479 contains several breaking changes for jQuery UI

The first is the switch to return undefined for hidden or disconnected elements which has broken all of our interactions tests.

The second is to allow offest to throw getBoundingRect not a function when offset is called on the window. While i agree this is probably not a valid use to begin with this may break a lot of other code expecting undefined

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 13, 2015

Member

For the first case, I think undefined is more accurate. Retrieving offset on a disconnected element doesn't make much sense when you think about it.

For the second, I'd be in support of failing silently and returning undefined.

Member

timmywil commented May 13, 2015

For the first case, I think undefined is more accurate. Retrieving offset on a disconnected element doesn't make much sense when you think about it.

For the second, I'd be in support of failing silently and returning undefined.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 13, 2015

Member

The nice thing about returning undefined is that a naive caller will be expecting an object and die in their own code, which will be easier to debug. We don't document that it returns anything but an object and I'm okay with that since the unhandled cases are mentioned in the docs.

Member

dmethvin commented May 13, 2015

The nice thing about returning undefined is that a naive caller will be expecting an object and die in their own code, which will be easier to debug. We don't document that it returns anything but an object and I'm okay with that since the unhandled cases are mentioned in the docs.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 13, 2015

Member

Are the failures all from stuff like $( element || window ).offset()? Because if so, just use $( element ).offset() instead—the method will return undefined if its context collection is empty.

As part of the larger story, though, these are the kinds of things that I expect to see: failures from mild API misuse that are easy for consumers to fix. If we're not serious about valid vs. invalid input for our code, then I want to know now, because we'll just end up backing out every other change.

Member

gibson042 commented May 13, 2015

Are the failures all from stuff like $( element || window ).offset()? Because if so, just use $( element ).offset() instead—the method will return undefined if its context collection is empty.

As part of the larger story, though, these are the kinds of things that I expect to see: failures from mild API misuse that are easy for consumers to fix. If we're not serious about valid vs. invalid input for our code, then I want to know now, because we'll just end up backing out every other change.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 13, 2015

Member

If we're not serious about valid vs. invalid input for our code, then I want to know now, because we'll just end up backing out every other change.

In this case, returning undefined is still one way to handle invalid input. Not everything needs to throw an error, right?

Member

timmywil commented May 13, 2015

If we're not serious about valid vs. invalid input for our code, then I want to know now, because we'll just end up backing out every other change.

In this case, returning undefined is still one way to handle invalid input. Not everything needs to throw an error, right?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 13, 2015

Member

@gibson042 yes it is from things like that and i do agree its easy to fix we will fix it regardless of the outcome. However the problem is this can be pretty buried in code and hard to track down.

Member

arschmitz commented May 13, 2015

@gibson042 yes it is from things like that and i do agree its easy to fix we will fix it regardless of the outcome. However the problem is this can be pretty buried in code and hard to track down.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 13, 2015

Member

Also you return undefined on disconnected or hidden nodes why would you throw on window but return undefined in those cases returning undefined in any "invalid" case would seem better.

Member

arschmitz commented May 13, 2015

Also you return undefined on disconnected or hidden nodes why would you throw on window but return undefined in those cases returning undefined in any "invalid" case would seem better.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 13, 2015

Member

In this case, returning undefined is still one way to handle invalid input. Not everything needs to throw an error, right?

I don't want to "handle" invalid input. We're not creating errors, we're exposing them by assuming that the input is valid. Why check for conditions that we tell consumers to avoid creating? Or in other words, how can input be invalid if we explicitly check for it?

Member

gibson042 commented May 13, 2015

In this case, returning undefined is still one way to handle invalid input. Not everything needs to throw an error, right?

I don't want to "handle" invalid input. We're not creating errors, we're exposing them by assuming that the input is valid. Why check for conditions that we tell consumers to avoid creating? Or in other words, how can input be invalid if we explicitly check for it?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 13, 2015

Member

how can input be invalid if we explicitly check for it?

I see your point.

Member

timmywil commented May 13, 2015

how can input be invalid if we explicitly check for it?

I see your point.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 13, 2015

Member

you return undefined on disconnected or hidden nodes why would you throw on window

Well, looking at it from the other side, they are slightly different. Disconnected elements still have getBoundingClientRect(), while window does not. So, the current behavior reflects native in that way.

Member

timmywil commented May 13, 2015

you return undefined on disconnected or hidden nodes why would you throw on window

Well, looking at it from the other side, they are slightly different. Disconnected elements still have getBoundingClientRect(), while window does not. So, the current behavior reflects native in that way.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 13, 2015

Member

That's a good point, @arschmitz. I guess I consider any element to be valid input, but the output is literally undefined if they have no layout boxes.

Member

gibson042 commented May 13, 2015

That's a good point, @arschmitz. I guess I consider any element to be valid input, but the output is literally undefined if they have no layout boxes.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 13, 2015

Member

Another side of this is when you release 3.0 if ui.1.12 is not out yet current stable jQuery will not work with any version of UI. I'm not sure if you really care about that or not though.

Member

arschmitz commented May 13, 2015

Another side of this is when you release 3.0 if ui.1.12 is not out yet current stable jQuery will not work with any version of UI. I'm not sure if you really care about that or not though.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 13, 2015

Member

On the bright side, returning undefined makes the error happen on the caller's side if they're not expecting it. If this was some minor plugin or rare user mistake I'd say let's leave it as-is, but breaking all current and past UI is gonna make a mess.

Member

dmethvin commented May 13, 2015

On the bright side, returning undefined makes the error happen on the caller's side if they're not expecting it. If this was some minor plugin or rare user mistake I'd say let's leave it as-is, but breaking all current and past UI is gonna make a mess.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez May 13, 2015

Member

Are the failures all from stuff like $( element || window ).offset()? Because if so, just use $( element ).offset() instead—the method will return undefined if its context collection is empty.

The code @arschmitz showed was simplified. In reality, what happens is we are determining which element to operate on, and the default element is the window. Later on, we get the offset of the element, and that's where we're getting the error. We already have guards against windows in other parts of the code, so adding another one is fine.

Member

scottgonzalez commented May 13, 2015

Are the failures all from stuff like $( element || window ).offset()? Because if so, just use $( element ).offset() instead—the method will return undefined if its context collection is empty.

The code @arschmitz showed was simplified. In reality, what happens is we are determining which element to operate on, and the default element is the window. Later on, we get the offset of the element, and that's where we're getting the error. We already have guards against windows in other parts of the code, so adding another one is fine.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 13, 2015

Member

Also, the nature of the implementation is such that if no-layout elements were ignored as truly invalid input, then the output would be indistinguishable from valid input (since gBCR returns { top: 0, left: 0 }, which coincides with elements located at the origin).

Member

gibson042 commented May 13, 2015

Also, the nature of the implementation is such that if no-layout elements were ignored as truly invalid input, then the output would be indistinguishable from valid input (since gBCR returns { top: 0, left: 0 }, which coincides with elements located at the origin).

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 13, 2015

Member

@scottgonzalez yeah like i said i think we should fix this regardless of if core backs out the change or not

Member

arschmitz commented May 13, 2015

@scottgonzalez yeah like i said i think we should fix this regardless of if core backs out the change or not

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 13, 2015

Member

@gibson042 I'd just like to point out that I do not disagree with you on how invalid input should be handled. The problem is figuring out how much we can change right now without causing too much of a ruckus. So, I wouldn't interpret any change here as a policy change to be applied throughout the codebase.

@arschmitz Is this the only breaking change you've noticed so far in regards to how well previous versions of UI work with jQuery 3.0?

Member

timmywil commented May 13, 2015

@gibson042 I'd just like to point out that I do not disagree with you on how invalid input should be handled. The problem is figuring out how much we can change right now without causing too much of a ruckus. So, I wouldn't interpret any change here as a policy change to be applied throughout the codebase.

@arschmitz Is this the only breaking change you've noticed so far in regards to how well previous versions of UI work with jQuery 3.0?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 13, 2015

Member

@timmywil yes i think this is the only breaking change within the actual code at this point. @scottgonzalez are you aware of any others? I think a couple had come up but got backed out.

Member

arschmitz commented May 13, 2015

@timmywil yes i think this is the only breaking change within the actual code at this point. @scottgonzalez are you aware of any others? I think a couple had come up but got backed out.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 13, 2015

Member

Sorry if I came across too strong here, but I was thinking of precisely those other changes that got reverted. The feedback from jQuery UI is valuable, and in some cases I can see us expanding our domain of valid input to support broader use... maybe this is one of them. But I wouldn't consider it unreasonable for UI <=1.12 UI to break on Core 3.0 without Migrate. That's the very purpose of Migrate, and in my opinion it allows us the flexibility to move forward while preserving temporary backwards compatibility.

I just don't want to see us stuck because a particularly important and still healthy downstream consumer happens to currently misuse some functionality.

Member

gibson042 commented May 13, 2015

Sorry if I came across too strong here, but I was thinking of precisely those other changes that got reverted. The feedback from jQuery UI is valuable, and in some cases I can see us expanding our domain of valid input to support broader use... maybe this is one of them. But I wouldn't consider it unreasonable for UI <=1.12 UI to break on Core 3.0 without Migrate. That's the very purpose of Migrate, and in my opinion it allows us the flexibility to move forward while preserving temporary backwards compatibility.

I just don't want to see us stuck because a particularly important and still healthy downstream consumer happens to currently misuse some functionality.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 13, 2015

Member

@gibson042 so are you saying migrate will fix this because at least right now it does not?

Member

arschmitz commented May 13, 2015

@gibson042 so are you saying migrate will fix this because at least right now it does not?

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez May 13, 2015

Member

But I wouldn't consider it unreasonable for UI <=1.12 UI to break on Core 3.0 without Migrate.

Agreed. We can handle this in UI regardless. We can also push out 1.11.5 with support for Core 3.0. We have a history of going back and doing unexpected releases to get support for new versions of core since it eases the upgrade path for users.

Member

scottgonzalez commented May 13, 2015

But I wouldn't consider it unreasonable for UI <=1.12 UI to break on Core 3.0 without Migrate.

Agreed. We can handle this in UI regardless. We can also push out 1.11.5 with support for Core 3.0. We have a history of going back and doing unexpected releases to get support for new versions of core since it eases the upgrade path for users.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 14, 2015

Member

I'd say if this caused so much problems with UI, it has to cause a lot of problems in the user-land.

I would go through the safest way possible - in the blog post, explicitly mention incorrect use of this API, document it (yeah, sounds weird), put warnings into migrate and do this only in next major.

Technically speaking, @gibson042 is absolutely right, but practically, this way is too dangerous for my taste.

Member

markelog commented May 14, 2015

I'd say if this caused so much problems with UI, it has to cause a lot of problems in the user-land.

I would go through the safest way possible - in the blog post, explicitly mention incorrect use of this API, document it (yeah, sounds weird), put warnings into migrate and do this only in next major.

Technically speaking, @gibson042 is absolutely right, but practically, this way is too dangerous for my taste.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 14, 2015

Member

Just for the record this is breaking things on mobile as well though not nearly to the extent of #2300

Member

arschmitz commented May 14, 2015

Just for the record this is breaking things on mobile as well though not nearly to the extent of #2300

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 15, 2015

Member

@arschmitz Would mobile's breakages from #2300 be addressed by #2303 as it stands?

Member

timmywil commented May 15, 2015

@arschmitz Would mobile's breakages from #2300 be addressed by #2303 as it stands?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 18, 2015

Member

Behavior for invalid input is undefined. Since getting offset on a window is not defined behavior, it may throw an error or it may not. We're going to let migrate take over on this one. If it turns out that this causes too much of an uproar in user code, we can address that after beta release.

Member

timmywil commented May 18, 2015

Behavior for invalid input is undefined. Since getting offset on a window is not defined behavior, it may throw an error or it may not. We're going to let migrate take over on this one. If it turns out that this causes too much of an uproar in user code, we can address that after beta release.

@timmywil timmywil closed this May 18, 2015

@timmywil timmywil removed the Needs review label May 18, 2015

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 18, 2015

Member

I think letting migrate handle this seems like a good way to go. I agree calling on window does not make sense.

@timmywil i believe #2300 is addressed for us by #2303 but @gabrielschulhof would know better since he debugged that issue and submitted the patch.

Member

arschmitz commented May 18, 2015

I think letting migrate handle this seems like a good way to go. I agree calling on window does not make sense.

@timmywil i believe #2300 is addressed for us by #2303 but @gabrielschulhof would know better since he debugged that issue and submitted the patch.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof May 18, 2015

Contributor

@arschmitz @timmywil I'll test my PR flat-out against mobile.

Contributor

gabrielschulhof commented May 18, 2015

@arschmitz @timmywil I'll test my PR flat-out against mobile.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof May 18, 2015

Contributor

@arschmitz @timmywil Actually, with my fix, 7 more assertions fail :)

Contributor

gabrielschulhof commented May 18, 2015

@arschmitz @timmywil Actually, with my fix, 7 more assertions fail :)

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 19, 2015

Member

@timmywil so most of this issue discussion has been about the change to $( window ).offset() because i thought the changes in the first part of my opening comment about returning undefined on hidden or detached elements was in tests only. However i was wrong and i think this has more potential to break things in userland then the change for how window is handled.

This comes up in ui and mobile quite a bit it turns out. Both jQuery UI and Mobile allow widgets to be instantiated on a default element in a detached state via $.ui[widgetname]() and also just in general allow instantiating widgets on detached elements for improved performance among other reasons.

There are also many many situations where a widget or other plugin of some sort may be instantiated or called inside of a hidden container. This is a major issue for plugin authors that cant ever reasonably know if they may be inside a hidden container ( think popups panels etc ) or in jQuery Mobile everything is hidden when the page is initialized.

This all means where as in the past you could call offset() on any element with out any worry you now need to always check the return value before using it. So it will make a lot of code go from

var left = $( elem ).offset().left

to

var offset = $( elem ).offset();
var left = offset ? offset.left || 0;

Who knows it would not surprise me if you start seeing things where offset is used a lot, doing something like the below to just not have to worry about it.

$.fn.safeOffset = function() { return this.offset() || { top: 0, left: 0 }; };

Because with this change it essentially makes it ( as a plugin author ) never safe to use offset with out first checking its return value, unless it is in direct response to a user action, that essentially cant happen on hidden or detached elements. for example dragging and mouse interaction type stuff ( this is a case for many of the cases of offset in ui )

Member

arschmitz commented May 19, 2015

@timmywil so most of this issue discussion has been about the change to $( window ).offset() because i thought the changes in the first part of my opening comment about returning undefined on hidden or detached elements was in tests only. However i was wrong and i think this has more potential to break things in userland then the change for how window is handled.

This comes up in ui and mobile quite a bit it turns out. Both jQuery UI and Mobile allow widgets to be instantiated on a default element in a detached state via $.ui[widgetname]() and also just in general allow instantiating widgets on detached elements for improved performance among other reasons.

There are also many many situations where a widget or other plugin of some sort may be instantiated or called inside of a hidden container. This is a major issue for plugin authors that cant ever reasonably know if they may be inside a hidden container ( think popups panels etc ) or in jQuery Mobile everything is hidden when the page is initialized.

This all means where as in the past you could call offset() on any element with out any worry you now need to always check the return value before using it. So it will make a lot of code go from

var left = $( elem ).offset().left

to

var offset = $( elem ).offset();
var left = offset ? offset.left || 0;

Who knows it would not surprise me if you start seeing things where offset is used a lot, doing something like the below to just not have to worry about it.

$.fn.safeOffset = function() { return this.offset() || { top: 0, left: 0 }; };

Because with this change it essentially makes it ( as a plugin author ) never safe to use offset with out first checking its return value, unless it is in direct response to a user action, that essentially cant happen on hidden or detached elements. for example dragging and mouse interaction type stuff ( this is a case for many of the cases of offset in ui )

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof May 19, 2015

Contributor

The fact that .offset() used as a setter also crashes is bad because the only way to avoid the crash when calling the setter is to find out first whether the element is disconnected/hidden:

if ( this.element.offset() ) {
  this.element.offset( {
    left: desiredLeftCoordinate,
    top: desiredTopCoordinate
  } );
}

This code is hugely bad when the element is not disconnected, because querying and setting coordinates in close succession causes reflows.

Contributor

gabrielschulhof commented May 19, 2015

The fact that .offset() used as a setter also crashes is bad because the only way to avoid the crash when calling the setter is to find out first whether the element is disconnected/hidden:

if ( this.element.offset() ) {
  this.element.offset( {
    left: desiredLeftCoordinate,
    top: desiredTopCoordinate
  } );
}

This code is hugely bad when the element is not disconnected, because querying and setting coordinates in close succession causes reflows.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof May 19, 2015

Contributor

Well, either the above or

try {
  this.element.offset( {
    left: desiredLeftCoordinate,
    top: desredTopCoordinate
  } );
} catch( anError ) {
}

This won't thrash the DOM, but I dunno about the try{} block ...

Contributor

gabrielschulhof commented May 19, 2015

Well, either the above or

try {
  this.element.offset( {
    left: desiredLeftCoordinate,
    top: desredTopCoordinate
  } );
} catch( anError ) {
}

This won't thrash the DOM, but I dunno about the try{} block ...

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 19, 2015

Member

@gabrielschulhof correct me if i'm wrong but that wont really work if you call on a collection, and one or more but not all elements are either hidden or disconnected. It will terminate as soon as it hits the first hidden or disconnected element and not continue on the rest of the collection?

Member

arschmitz commented May 19, 2015

@gabrielschulhof correct me if i'm wrong but that wont really work if you call on a collection, and one or more but not all elements are either hidden or disconnected. It will terminate as soon as it hits the first hidden or disconnected element and not continue on the rest of the collection?

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof May 19, 2015

Contributor

True! So, you need to do .each(), and try inside the callback.

Contributor

gabrielschulhof commented May 19, 2015

True! So, you need to do .each(), and try inside the callback.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 19, 2015

Member

@timmywil it seems we need to discuss this further?

Member

markelog commented May 19, 2015

@timmywil it seems we need to discuss this further?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 19, 2015

Member

@arschmitz @gabrielschulhof That's a good point. Checking if an element is disconnected before setting does seem too expensive. Would you be comfortable with continuing to allow $(window).offset() to throw, but returning zeros for disconnected elements? While it doesn't necessarily make sense to get offset on disconnected elements, the behavior would actually be more in line with native gBCR.

Member

timmywil commented May 19, 2015

@arschmitz @gabrielschulhof That's a good point. Checking if an element is disconnected before setting does seem too expensive. Would you be comfortable with continuing to allow $(window).offset() to throw, but returning zeros for disconnected elements? While it doesn't necessarily make sense to get offset on disconnected elements, the behavior would actually be more in line with native gBCR.

@timmywil timmywil reopened this May 19, 2015

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 19, 2015

Member

@gabrielschulhof

This code is hugely bad when the element is not disconnected, because querying and setting coordinates in close succession causes reflows.

That's already an inherent part of the offset setter:

curOffset = curElem.offset();

@arschmitz

Who knows it would not surprise me if you start seeing things where offset is used a lot, doing something like the below to just not have to worry about it.

$.fn.safeOffset = function() { return this.offset() || { top: 0, left: 0 }; };

$el.offset() || { top: 0, left: 0 } (explicitly providing default data for later code, just like the old behavior) is entirely appropriate and expected in library/plugin code. In fact, it's what we (very) briefly had in the setter between 1617479 and 0d11c11.

if you call on a collection, and one or more but not all elements are either hidden or disconnected. It will terminate as soon as it hits the first hidden or disconnected element and not continue on the rest of the collection?

This, on the other hand, strikes me as a problem, and may be sufficient reason to back out 0d11c11. @arschmitz, @gabrielschulhof: how often do you simultaneously set the offset of more than one element?

Member

gibson042 commented May 19, 2015

@gabrielschulhof

This code is hugely bad when the element is not disconnected, because querying and setting coordinates in close succession causes reflows.

That's already an inherent part of the offset setter:

curOffset = curElem.offset();

@arschmitz

Who knows it would not surprise me if you start seeing things where offset is used a lot, doing something like the below to just not have to worry about it.

$.fn.safeOffset = function() { return this.offset() || { top: 0, left: 0 }; };

$el.offset() || { top: 0, left: 0 } (explicitly providing default data for later code, just like the old behavior) is entirely appropriate and expected in library/plugin code. In fact, it's what we (very) briefly had in the setter between 1617479 and 0d11c11.

if you call on a collection, and one or more but not all elements are either hidden or disconnected. It will terminate as soon as it hits the first hidden or disconnected element and not continue on the rest of the collection?

This, on the other hand, strikes me as a problem, and may be sufficient reason to back out 0d11c11. @arschmitz, @gabrielschulhof: how often do you simultaneously set the offset of more than one element?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 19, 2015

Member

Would you be comfortable with continuing to allow $(window).offset() to throw, but returning zeros for disconnected elements?

Yes this would be what i would recommend the window change should be rare and is very very simple to fix, so as long as its handled by migrate i see no issue. The issues with offset on hidden elements specifically seems like a much bigger problem and honestly will make offset a PITA to use.

How often do you simultaneously set the offset of more than one element?

Im not aware of any case in either library where this is done ( i can only think of one case where i did this ever ) it was just the first thing i thought of looking at the code because the offset documentation says "Set the current coordinates of every element in the set of matched elements, relative to the document." specifically mentioning collections being supported.

the behavior would actually be more in line with native gBCR

This seems like a valid point to me.

Member

arschmitz commented May 19, 2015

Would you be comfortable with continuing to allow $(window).offset() to throw, but returning zeros for disconnected elements?

Yes this would be what i would recommend the window change should be rare and is very very simple to fix, so as long as its handled by migrate i see no issue. The issues with offset on hidden elements specifically seems like a much bigger problem and honestly will make offset a PITA to use.

How often do you simultaneously set the offset of more than one element?

Im not aware of any case in either library where this is done ( i can only think of one case where i did this ever ) it was just the first thing i thought of looking at the code because the offset documentation says "Set the current coordinates of every element in the set of matched elements, relative to the document." specifically mentioning collections being supported.

the behavior would actually be more in line with native gBCR

This seems like a valid point to me.

@timmywil timmywil self-assigned this May 27, 2015

@timmywil timmywil added this to the 3.0.0 milestone May 27, 2015

@timmywil timmywil added the Offset label May 27, 2015

@timmywil timmywil added the Blocker label Jun 1, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Jun 15, 2015

timmywil added a commit to timmywil/jquery that referenced this issue Jun 15, 2015

@timmywil timmywil closed this in 40dcc76 Jun 16, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jun 1, 2016

Member

At the moment, there is no shipping version of UI that works with jQuery core 3.0.0 due to this change. You need Migrate which shims the {0, 0} behavior for$(window).offset(). Just adding here to be sure that everyone is okay with it. I would personally be more comfortable if the UI 1.11 branch had something that worked with 3.0. How long until UI 1.12 is ready?

As far as action items for core, if 1.12 isn't out we need to make it clear in the upgrade guide and blog post that people will need Migrate if they want to use UI.

Member

dmethvin commented Jun 1, 2016

At the moment, there is no shipping version of UI that works with jQuery core 3.0.0 due to this change. You need Migrate which shims the {0, 0} behavior for$(window).offset(). Just adding here to be sure that everyone is okay with it. I would personally be more comfortable if the UI 1.11 branch had something that worked with 3.0. How long until UI 1.12 is ready?

As far as action items for core, if 1.12 isn't out we need to make it clear in the upgrade guide and blog post that people will need Migrate if they want to use UI.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Jun 1, 2016

Member

The last 1.12 RC will likely be released next week, with the final release probably a few weeks after that.

Member

scottgonzalez commented Jun 1, 2016

The last 1.12 RC will likely be released next week, with the final release probably a few weeks after that.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 1, 2016

Member

@scottgonzalez Is there any plan for a hotfix making UI 1.11 compatible with jQuery 3.0? Would that be hard to achieve?

Member

mgol commented Jun 1, 2016

@scottgonzalez Is there any plan for a hotfix making UI 1.11 compatible with jQuery 3.0? Would that be hard to achieve?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 1, 2016

Member

Worth noting 3.0 will also not work with any current release of jQuery Mobile without migrate. JQM is further out the UI but also planning a release soon. We are planning a patch for 1.4 to support 3.0 but its going to be a bit because there were a lot of breaking changes for mobile we need to sort out which need to be patched

Member

arschmitz commented Jun 1, 2016

Worth noting 3.0 will also not work with any current release of jQuery Mobile without migrate. JQM is further out the UI but also planning a release soon. We are planning a patch for 1.4 to support 3.0 but its going to be a bit because there were a lot of breaking changes for mobile we need to sort out which need to be patched

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 1, 2016

Member

Is there any plan for a hotfix making UI 1.11 compatible with jQuery 3.0? Would that be hard to achieve?

This would be simple to patch and would probably cherry-pick cleanly even ( I fixed this in UI ) not sure about release and timing though

Member

arschmitz commented Jun 1, 2016

Is there any plan for a hotfix making UI 1.11 compatible with jQuery 3.0? Would that be hard to achieve?

This would be simple to patch and would probably cherry-pick cleanly even ( I fixed this in UI ) not sure about release and timing though

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Jun 9, 2016

Member

@scottgonzalez Is there any plan for a hotfix making UI 1.11 compatible with jQuery 3.0? Would that be hard to achieve?

I'm looking at what's involved in a 1.11.5 release right now. So far I've narrowed down 600+ commits to under 200 to be reviewed for inclusion in the release. I'll work through that list and then test against jQuery git.

Member

scottgonzalez commented Jun 9, 2016

@scottgonzalez Is there any plan for a hotfix making UI 1.11 compatible with jQuery 3.0? Would that be hard to achieve?

I'm looking at what's involved in a 1.11.5 release right now. So far I've narrowed down 600+ commits to under 200 to be reviewed for inclusion in the release. I'll work through that list and then test against jQuery git.

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

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