Bug Tracker

Opened 8 years ago

Closed 8 years ago

Last modified 8 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 8 years ago by dmethvin

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

Confirmed.

comment:2 Changed 8 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 8 years ago by tomgrohl (previous) (diff)

comment:3 Changed 8 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 8 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 8 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 8 years ago by tomgrohl

Yes sure I'll get on it.

comment:7 Changed 8 years ago by tomgrohl

Pull request here

Includes Unit Tests

comment:8 Changed 8 years ago by Rick Waldron

Please see pull request comments.

comment:9 Changed 8 years ago by timmywil

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 8 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 8 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 8 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 8 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 8 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 8 years ago by thomas.jaggi@…

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

comment:16 Changed 8 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.