Bug 14893 – WebKit confuses width/height for Media Queries device-aspect-ratio evaluation

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oliver Hunt]

In WebCore/css/MediaQueryEvaluator.cpp the method device_aspect_ratioMediaFeatureEval() confuses width and height in line 241.

As a result, the MQ evaluation of "device-aspect-ratio" always fails.

To correct the bug, line 241 must read:

          return b != 0  && cmpvalue(a * (int)sg.width(), b * (int)sg.height(), op);

<rdar://problem/5380295>

[Oliver Hunt]

Created attachment 15853 [details]
PAtch for bug

Patch from ADC developer

[Darin Adler]

Comment on attachment 15853 [details]
PAtch for bug

Fix looks fine. Needs a regression test.

[David Kilzer (:ddkilzer)]

Reassigning to webkit-unassigned for more visibility.

All we need is a test case to land this fix.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Reassigning to webkit-unassigned for more visibility.
> 
> All we need is a test case to land this fix.

Actually, I believe the current code is correct (but with poorly-named variables).  We should be cross-multiplying the values to compare them, which the current code does.

http://www.w3.org/TR/css3-mediaqueries/#device-aspect-ratio

I'm going to write test cases for [min-|max-|]device-aspect-ratio and fix the variable names.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Actually, I believe the current code is correct (but with poorly-named
> variables).  We should be cross-multiplying the values to compare them, which
> the current code does.
> 
> http://www.w3.org/TR/css3-mediaqueries/#device-aspect-ratio

The current code is incorrect, but so is the patch (Attachment #15853 [details]).  The entire first and second arguments need to be swapped, not just the sg.width() and sg.height() calls.

> I'm going to write test cases for [min-|max-|]device-aspect-ratio and fix the
> variable names.

Test cases are a Good Thing(tm).

[David Kilzer (:ddkilzer)]

Created attachment 17078 [details]
Patch v2

Now with correct fix and 3 layout tests.

[Darin Adler]

Comment on attachment 17078 [details]
Patch v2

r=me

(I probably would have used h/v instead of longer names.)

[David Kilzer (:ddkilzer)]

The test cases also pass on Opera 9.50b and Firefox 2.0.0.9 on Tiger.  (Note that the shouldBe() test fails on Opera 9.50b because computed styles are accessed differently than Safari/Firefox.  The text turns green as it is supposed to, though.)

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Note
> that the shouldBe() test fails on Opera 9.50b because computed styles are
> accessed differently than Safari/Firefox.

Filed Bug 15888 to provide cross-browser method for getComputedStyle().

[David Kilzer (:ddkilzer)]

Committed revision 27581.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (I probably would have used h/v instead of longer names.)

I made this change before committing.