Bug Tracker

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9948 closed bug (wontfix)

RegExp to test for pixel values might be unreliable in IE 9

Reported by: thomas.jaggi@… Owned by:
Priority: high Milestone: 1.7
Component: css Version: 1.6.2
Keywords: Cc:
Blocked by: Blocking:

Description

IE 9 (tested in version 9.0.8112 on Windows 7) does not seem to return rounded decimal pixel values when using currentStyle. Therefore, the regular expression used to detect pixel values (/-?\d+(?:px)?$/i) is not working properly there: http://jsfiddle.net/YPDHF/1/

Change History (16)

comment:1 Changed 12 years ago by dmethvin

Component: unfiledcss
Milestone: None1.7
Priority: undecidedhigh
Status: newopen

Confirmed.

comment:2 Changed 12 years ago by tomgrohl

Tested a fix. Added an example here: http://jsfiddle.net/tomgrohl/Accr3/

Contains the fix:

newrnumpx = /^-?\d+(?:\.\d+)?(?:px)?$/i

Which works in IE9 for both 3px and 3.14px.

@dmethvin if your happy with this I can create a pull request.

Last edited 12 years ago by tomgrohl (previous) (diff)

comment:3 Changed 12 years ago by thomas.jaggi@…

@tomgrohl: Thanks for the additional tests! However, If I remember it correctly, the corresponding code block was only relevant for IE.

comment:4 Changed 12 years ago by tomgrohl

@thomas.jaggi@…: Yes true. I'll amended comment

The fix is still relevant for IE though, which is good.

comment:5 Changed 12 years ago by dmethvin

Cc: tomgrohl added

@tomgrohl, if you could create a pull request that would be great! Can we have a unit test case as well?

comment:6 Changed 12 years ago by tomgrohl

Yes sure I'll get on it.

comment:7 Changed 12 years ago by tomgrohl

Pull request here

Includes Unit Tests

comment:8 Changed 12 years ago by Rick Waldron

Please see pull request comments.

comment:9 Changed 12 years ago by Timmy Willison

Cc: tomgrohl removed
Keywords: needsdocs added
Resolution: wontfix
Status: openclosed

Thanks for your work. Since not all browsers (including webkit and firefox) support decimal pixel values, I think we should simply suggest that users stick with integer values. It keeps the code simpler and smaller.

comment:10 Changed 12 years ago by tomgrohl

@timmywil

I agree, as I was I only looking into fixing it for IE only Iassumed it would work in other browsers. How wrong I was, lol.

comment:11 Changed 12 years ago by thomas.jaggi@…

I disagree. First of all, the respective code block is relevant for IE only anyway, as far as I remember. And I'd say this is not about 'sticking with integer values' since it happens when computing the size of something set in relative units l(ike em or %) which does not correspond to an integer px value.

comment:12 Changed 12 years ago by thomas.jaggi@…

Well, to be fair, I should probably test that first, but I'm on my iPhone right know... But I think I stumbled upon this when trying to get the font size of an element set in em.

comment:13 Changed 12 years ago by tomgrohl

It has been found that it is an issue in other browsers, not just IE.

The problem is that the behaviour is not consistent, and would not be possible to fix.

comment:14 Changed 12 years ago by dmethvin

Oh man this test was totally off base and I should have caught it.

First, there is no jQuery in the test case, it's got code snipped from places *inside* jQuery but nothing showing that jQuery is doing anything wrong in a real-life case.

Second, IE9 supports getComputedStyle so it doesn't even go through the currentStyle section of code -- another hazard of code clipping rather than a real test case.

Third, several browsers including Firefox and IE9 return fractional pixels from getComputedStyle and we appear to round properly to the nearest pixel when returning those values:

http://jsfiddle.net/tY8Cd/3/

So this ticket seems more like invalid or worksforme, there's nothing yet shown that needs fixing.

comment:15 Changed 12 years ago by thomas.jaggi@…

Argh, sorry for wasting your time, I obviously didn't think that through.

comment:16 Changed 12 years ago by addyosmani

Keywords: needsdocs removed

Confirmed with timmywil we can remove the needsdocs on this one.

Note: See TracTickets for help on using tickets.