Bug Tracker

Ticket #7730 (closed bug: fixed)

Opened 4 years ago

Last modified 3 years ago

offset.js: setOffset uses parseInt to parse css values which may contain floating point numbers

Reported by: inukshuk Owned by: inukshuk
Priority: high Milestone: 1.6
Component: css Version: 1.4.4
Keywords: Cc: scott.gonzalez
Blocking: Blocked by:

Description

I encountered 'wobbly' behaviour using jQuery UI autocomplete, caused by sub-pixel rendering issues.

Use parseFloat instead; see commit:

 https://github.com/inukshuk/jquery/commit/4b0b05ffc8a087b8053f08099511f4bf2490939f

Change History

comment:1 Changed 4 years ago by scott.gonzalez

  • Cc scott.gonzalez added

comment:2 Changed 4 years ago by rwaldron

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

Thanks for taking the time to contribute to the jQuery project! Please provide a reduced jsFiddle test case to help us assess your ticket!

Additionally, test against the latest jQuery release AND the jQuery 0 GIT version.

In this case, please provide a test that demonstrates the "wobbly" behaviour you're describing. Thanks!

comment:3 Changed 4 years ago by ajpiano

parseFloat does not accept a radix argument.

comment:4 Changed 4 years ago by inukshuk

  • Status changed from pending to new

Thanks for taking a look at this. The 'wobbling' effect can be encountered if a function repeatedly positions an element using setOffset, for example the autocomplete widget in jQuery UI.

The problem is that it is possible, given the right circumstances, for fractional values to be written to CSS -- this value is then parsed by the second call to setOffset and ommitted if parseInt is used for that purpose. The effect is that the element in question will be alternating between a position based on float and one on int. Whether or not this results in a visible bug depends on the exact numbers invovled and on the browser's sub-pixel rendering I would think. It is therefore difficult to produce a minimal example, but you can observe the behaviour I describe here:

 http://jsfiddle.net/gy9KU/

You need to monitor the CSS top value of the (initially hidden) ui-autocomplete div.

  1. Select the input field and type 'a'; ui-autocomplete is displayed using a top value of '-2.39999px'.
  1. Now, clear the input field and type 'a' again; ui-autocomplete is displayed, this time using a top value of '-2px'.

You can repeat these steps indefinitely with the same behaviour.

I encountered this on a site, where the fraction in question was > 0.5 and the result was plainly visible in many browsers. I suspect it is the problem that caused these issues in jQuery UI:

 http://bugs.jqueryui.com/ticket/5280  http://bugs.jqueryui.com/ticket/6000

If I use parseFloat in setOffset I can remove the superflous Math.round or parseInt calls in ui-position. In the case of ui-position I believe this is the best solution, because ui-position has no way of knowing what a browser will do with fractions, therefore it should not touch them either way.

Of course you're right about the radix argument; here is a commit without it:

 https://github.com/inukshuk/jquery/commit/25b509ec755bfd875c1ccb3e0a9ca182e49a156c

comment:5 Changed 4 years ago by snover

  • Status changed from new to pending

Could you please provide a pull request for your patch on GitHub along with a unit test? Thanks!

comment:6 Changed 4 years ago by inukshuk

  • Status changed from pending to new

Do you have any suggestions for a suitable unit test? The best test case I can think of is to set a sub-pixel value in CSS and test that offset() corectly parses the fractions. But what if the browser does not report the sub-pixels? Is there any policy to deal with browser differences in the unit tests? I guess they are best avoided, but then, how best to test this issue?

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

comment:7 Changed 4 years ago by snover

So, if I’m understanding this issue correctly, the problem with parseInt for most browsers is that it doesn’t round—it just drops the fractional amount—which means browsers that use round instead of floor when deciding how to place fractional pixels end up jumping.

What might work as a reasonable test case would be to use a value that doesn’t round properly currently (like 1.7), set & get it, then Math.round and compare. Incorrect behaviour would end up with 1; correct behaviour would end up with 2. Right? Are there browsers that actually use floor behaviour?

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

comment:8 Changed 4 years ago by snover

  • Priority changed from undecided to high
  • Status changed from new to open
  • Component changed from unfiled to css
  • Milestone changed from 1.next to 1.5

comment:9 Changed 4 years ago by inukshuk

As I understand it browsers may legitimately use sub-pixel values. There are some problems with this, however. Consider a case where you have three columns in a container and each column is designed to take up a third of the container. Now, what happens if the container's witdh divided by 3 results in a fractional value and the browser or display does not support sub-pixel rendering? The browser has to pick the lesser of three evils:

  1. make one column larger/smaller than the other to
  2. size the columns identically, but not fill the container
  3. size the columns identically, exceeding the container

This issue is also discussed here:

 http://ejohn.org/blog/sub-pixel-problems-in-css/

I think a Javascript library should handle all pixel values reported by the browser with utmost care; if the browser reports fractions we should not touch them, as I suppose it is conceivable for the scenarios to arise where the Browser decides to round up a value < 0.5 or vice versa. If the browser reports a fractional value, I think it is best to conserve it and feed it back to let the browser decide what to do with it.

Perhaps somebody with experience in sub-pixel rendering could verify this?

comment:10 Changed 4 years ago by inukshuk

I think the jsFiddle example in #7885 is actually a great test case. Using that, here is my commit again:

 https://github.com/jquery/jquery/pull/182

comment:11 Changed 3 years ago by john

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

Landed.

Note: See TracTickets for help on using tickets.