Opened 6 years ago

Closed 5 years ago

#10624 closed bug (notabug)

Position: alignments passed to "using" handler are unreliable

Reported by: r.otten Owned by: r.otten
Priority: minor Milestone: none
Component: ui.position Version: 1.11.1
Keywords: Cc:
Blocked by: Blocking:

Description

Currently the logic that sets the horizontal and vertical alignment passed to a using handler function is as follows:

horizontal: right < 0 ? "left" : left > 0 ? "right" : "center"
vertical: bottom < 0 ? "top" : top > 0 ? "bottom" : "middle"

https://github.com/jquery/jquery-ui/blob/9bb51d308e0a72bc67db948e92345c826a8f724d/ui/position.js#L293-L294

This is susceptible to rounding errors or errors related to numeric stability in browsers that report sub-pixel offsets and/or widths. In these browsers it may occur that the left, right, top or bottom values deviate from 0 by a minute sub-pixel value, leading to selection of the wrong aligment.

E.g. When right is -0.1 and left is 200, then horizontal will be set to "left" while it should be set to "right".

I suggest either applying rounding or widening the tolerance of the comparisons to half a pixel, i.e. :

horizontal: right <= .5 ? "left" : left >= .5 ? "right" : "center",
vertical: bottom <= .5 ? "top" : top >= .5 ? "bottom" : "middle"

That should avert the problem.

Change History (6)

comment:1 Changed 6 years ago by tj.vantoll

Owner: set to r.otten
Status: newpending

Hi r.otten,

Thanks for taking the time to contribute to jQuery UI. Do you have a specific situation that actually replicates this problem? You can use this test case as a starting point: http://jsfiddle.net/tj_vantoll/KTzQV/

comment:2 Changed 6 years ago by r.otten

Status: pendingnew

Hi,

I found a reproduction; http://jsfiddle.net/KTzQV/12/

The issue reproduces atleast in Firefox 32, where it reports a "center" alignment, rather than the expected "right" alignment.

Note that it is very hard to predictably trigger a problem with numerical stability in floating point numbers, so I'm making thankful use of another bug here to induce a rounding error. That bug being the fact that jQuery UI gets its support test for offset fractions wrong when executed after a document has already loaded its body element; hence the $.ajax call for the script.

Once that bug becomes patched, I expect this issue to occur a lot less frequent. (But it will still occur at times. That's floating point for you...)

Last edited 6 years ago by r.otten (previous) (diff)

comment:3 Changed 6 years ago by Scott González

Status: newpending
Version: git1.11.1

Using that fiddle, I don't see any difference by accounting for a 0.5px margin of error: http://jsfiddle.net/KTzQV/13/ In fact, the values I'm getting show that right is already negative (though negative by less than 0.5).

Anyway, I'm not sure that being off by one pixel is going to make much of a difference here since the value can changed based on the width of the element being positioned anyway. http://view.jqueryui.com/master/tests/visual/position/position_feedback.html shows the values at every possible placement, it's just not possible to have a generic value be perfect in all cases.

comment:4 Changed 6 years ago by r.otten

Status: pendingnew

You're right; comparing against a 0.5px value does not work. A different approach, however, does:

if (( 1 > abs( left )) && ( 1 > abs( right ))) {
  feedback.horizontal = "center";
} else if (( right <= -1 ) && ( left >= 1 )) {
  feedback.horizontal = "center";
} else if ( 1 > abs( left ) || ( right <= -1 )) {
  feedback.horizontal = "left";
} else if ( 1 > abs( right ) || ( left >= 1 )) {
  feedback.horizontal = "right";
} else {
  feedback.horizontal = "center";
}

if (( 1 > abs( top )) && ( 1 > abs( bottom ))) {
  feedback.vertical = "middle";
} else if (( bottom <= -1 ) && ( top >= 1 )) {
  feedback.vertical = "middle";
} else if ( 1 > abs( top ) || ( bottom <= -1 )) {
  feedback.vertical = "top";
} else if ( 1 > abs( bottom ) || ( top >= 1 )) {
  feedback.vertical = "bottom";
} else {
  feedback.vertical = "middle";
}

The lines that follow, namely:

if ( targetWidth < elemWidth && abs( left + right ) < targetWidth ) {
  feedback.horizontal = "center";
}
if ( targetHeight < elemHeight && abs( top + bottom ) < targetHeight ) {
  feedback.vertical = "middle";
}

also have to be removed, as they are also subject to use of subpixel values and floating point rounding errors. However, with the above change to check for aligned edges, they are also no longer needed.

See http://jsfiddle.net/KTzQV/17/ for the problem no longer occuring in the reduced reproduction and http://jsfiddle.net/xh6uq2rg/4/ for the visual position test using the updated logic.

(NOTE: I'm still working on reducing the number of comparisons.)

[EDIT] Managed to reduce the number of cases and factored out a few common computations. See updated fiddle http://jsfiddle.net/xh6uq2rg/8/

Last edited 6 years ago by r.otten (previous) (diff)

comment:5 Changed 6 years ago by Scott González

Please just provide a pull request. Pasting large blocks of code into tickets makes it very difficult to track changes and have discussions.

comment:6 Changed 5 years ago by Scott González

Resolution: notabug
Status: newclosed

I still don't think that this is a legitimate bug. As mentioned earlier, the generic feedback positions can never be optimized, that's why the exact numeric values are provided as well.

Note: See TracTickets for help on using tickets.