Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#8710 closed bug (fixed)

Position: collision: "flip" doesn't work in some situations

Reported by: olejorgenb Owned by: olejorgenb
Priority: minor Milestone: 1.11.3
Component: ui.position Version: 1.9.0
Keywords: Cc:
Blocked by: Blocking:

Description

http://jsfiddle.net/q9h3A/ (slightly modified position testcase)

Resize the window as small as you can without getting vertical scrollbar (about the size of the gray rectangle)

Move the target element down, at some point the narrow high blue rectangle should flip, but it doesn't. The smaller one works.

Change History (27)

comment:1 Changed 4 years ago by scottgonzalez

  • Owner set to olejorgenb
  • Status changed from new to pending

This is working fine for me. Are you seeing the problem in certain browsers?

comment:2 Changed 4 years ago by olejorgenb

  • Status changed from pending to new

Strange.. I've tested in IE9, FF and chrome

This is what I'm getting: http://i.imgur.com/CaCa6.png The long one is supposed to flip in that case, right?

comment:3 Changed 4 years ago by olejorgenb

(If I increase the window height slightly in the above screenshot and press "trigger position" it flips as expected)

comment:4 Changed 4 years ago by scottgonzalez

  • Status changed from new to open

Yup, I'm seeing the problem now. It seems to only appear with certain heights and certain scrolling.

comment:5 Changed 4 years ago by scottgonzalez

  • Summary changed from 'flip' doesn't work for my: "right top", at: "right bottom" (in some situations) to Position: collision: "flip" doesn't work for my: "right top", at: "right bottom" (in some situations)

comment:6 Changed 4 years ago by scottgonzalez

#8733 is a duplicate of this ticket.

comment:7 Changed 4 years ago by Deocarlos

The problem occurs with me too. When i use the autocomplete and the overflow occurs on bottom page, the collision:flip have problems. With collision:fit the behavior is ok.

The problem occurs with the last version of jQuery and jQuery-ui released. WHen i use jquery-1.7.2.min.js and jquery-ui-1.8.20 the problem doesn't occurs.

With position FLIP is problem: http://s10.postimage.org/txakha1e1/position_flip.png

With position FIT is ok: http://s8.postimage.org/kyxeu6f85/position_fit.png

Last edited 4 years ago by Deocarlos (previous) (diff)

comment:8 Changed 4 years ago by olejorgenb

Reverting https://github.com/jquery/jquery-ui/commit/7f808b2047725cd8fde51a948cb4e5f5946c82e1 seems to fix my problem.

PS: For my needs I'd rather want a "don't flip unless it makes the _whole_ element fit" option than "flip if more of the element fits when flipped" but that is obviously use-case dependent

comment:9 Changed 4 years ago by Deocarlos

The new version of jQueryUI (1.9.1) already had this change, and in some cases he still have problems with flip :(

comment:10 Changed 4 years ago by olejorgenb

Reverting that commit fixed my problem (ie. that commit seems to have introduced the bug)

comment:11 Changed 4 years ago by tj.vantoll

  • Milestone changed from 1.10.0 to none

comment:12 Changed 4 years ago by meyertee

I've ran into that problem too, and for what it's worth, here's another simpler test case: http://jsfiddle.net/meyertee/twrLf/ You can see the flip-behavior changing depending on the amount it overflows at the bottom.

I've also identified https://github.com/jquery/jquery-ui/commit/7f808b2047725cd8fde51a948cb4e5f5946c82e1 as the reason. As Deocarlos suggests, the motivation seems to be "only flip if it actually fits at the flipped position". But the code doesn't do that.

comment:13 Changed 4 years ago by k_borchers

I have to say I don't think there is a bug here. In the original fiddle http://jsfiddle.net/q9h3A/, because the containment is the window, this only looks like a bug due to the framing done by jsfiddle. If you look at the exact same fiddle but using the full browser window http://jsfiddle.net/q9h3A/show, it works as expected.

As for the second fiddle http://jsfiddle.net/meyertee/twrLf/, it appears to also be working as expected. The red div flips because there is a space large enough to accommodate it (210px of padding and the element is 200px tall). If you change the padding to 200px or less, the flip doesn't happen.

comment:14 Changed 4 years ago by scottgonzalez

There's definitely a bug. In the first fiddle the problem is only visible when the document is larger than the window. So take the full window version and adjust your browser size until there's a scrollbar. In the second one, even though there's space to fit the element after a flip, more of the element is visible if it doesn't flip, so it shouldn't flip.

comment:15 Changed 4 years ago by meyertee

To clarify: in the second fiddle the red layer is positioned to the bottom of the green layer. When making the display area smaller by moving the fiddle-separator down, it should flip to the top (because there's enough space and more space to the top then). It doesn't flip though, only within a small range it jumps up and then down again.

I'm working on a fix, just need to get the local setup & unit tests to work..

comment:16 Changed 4 years ago by meyertee

I created a pull request which reverts the aforementioned commit, and also added unit tests to show that the code before that commit already did what it seemingly aimed to fix. If there is a another reason for the commit that I didn't see, maybe you could add it as a unit test to make its intention a bit more clear?

https://github.com/jquery/jquery-ui/pull/1071

comment:17 Changed 3 years ago by Aaronius

I'm also reproducing this bug in a separate environment and also pinpointed it to the same line of code meyertee pointed out.

comment:18 Changed 3 years ago by scottgonzalez

#10061 is a duplicate of this ticket.

comment:19 Changed 3 years ago by scottadmi

I encountered this bug related to the flip position (in my case center top/bottom, but same cause) and the culprit is certainly the line referenced by meyertee. In my build 1.10.4 it is line 444 of jquery.ui.position.js (and also in the latest beta). The reversion of commit 7f808b2 solves it, however a smaller fix is possible because the root cause is a single inverted greater than/less than comparison (which did not account for the fact that negative values indicate overlap in the preceding if-block while positives do in this one).

The current line below:

if ( ( position.top + myOffset + atOffset + offset) > overBottom && ( newOverTop > 0 || abs( newOverTop ) < overBottom ) ) {

can be corrected to this:

if ( ( position.top + myOffset + atOffset + offset) < overBottom && ( newOverTop > 0 || abs( newOverTop ) < overBottom ) ) {
Last edited 3 years ago by scottadmi (previous) (diff)

comment:20 Changed 3 years ago by scottgonzalez

  • Summary changed from Position: collision: "flip" doesn't work for my: "right top", at: "right bottom" (in some situations) to Position: collision: "flip" doesn't work in some situations

comment:21 Changed 3 years ago by scottgonzalez

#9680 is a duplicate of this ticket.

comment:22 Changed 2 years ago by Thomas Meyer

  • Resolution set to fixed
  • Status changed from open to closed

Position: Restore old flip collision handling

This reverts commit 7f808b2047725cd8fde51a948cb4e5f5946c82e1.

Fixes #8710 Ref gh-1071

Changeset: ebaaca7206cae201ec069dbaed85bc8d6beeab32

comment:23 Changed 2 years ago by Thomas Meyer

Position: Add unit tests for bug 8710

Ref #8710 Closes gh-1071

Changeset: 4de983c6d5eacbdc668c0b7280d9818dd6281a53

comment:24 Changed 2 years ago by scottgonzalez

  • Milestone changed from none to 1.12.0

comment:25 Changed 2 years ago by Thomas Meyer

Position: Restore old flip collision handling

This reverts commit 7f808b2047725cd8fde51a948cb4e5f5946c82e1.

Fixes #8710 Ref gh-1071 (cherry picked from commit ebaaca7206cae201ec069dbaed85bc8d6beeab32)

Changeset: 276cd5cd8cbef787328335a11ad19864242ccafd

comment:26 Changed 2 years ago by Thomas Meyer

Position: Add unit tests for bug 8710

Ref #8710 Closes gh-1071 (cherry picked from commit 4de983c6d5eacbdc668c0b7280d9818dd6281a53)

Changeset: 9db405798dd5fc8ee603991226916351ec2a0dda

comment:27 Changed 2 years ago by scottgonzalez

  • Milestone changed from 1.12.0 to 1.11.3
Note: See TracTickets for help on using tickets.