Position: alignments passed to "using" handler are unreliable

[r.otten]

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.

[tj.vantoll]

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/

[r.otten]

Hi,

I found a reproduction; ​http://jsfiddle.net/KTzQV/12/ Works (or rather; fails) 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...)

[Scott González]

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.

[r.otten]

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/

[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.

[Scott González]

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.