Bug 14893 - WebKit confuses width/height for Media Queries device-aspect-ratio evaluation
Summary: WebKit confuses width/height for Media Queries device-aspect-ratio evaluation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-08-06 21:08 PDT by Oliver Hunt
Modified: 2007-11-07 14:01 PST (History)
1 user (show)

See Also:


Attachments
PAtch for bug (785 bytes, patch)
2007-08-06 21:14 PDT, Oliver Hunt
darin: review-
Details | Formatted Diff | Diff
Patch v2 (14.00 KB, patch)
2007-11-06 20:15 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2007-08-06 21:08:30 PDT
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>
Comment 1 Oliver Hunt 2007-08-06 21:14:06 PDT
Created attachment 15853 [details]
PAtch for bug

Patch from ADC developer
Comment 2 Darin Adler 2007-08-11 15:42:39 PDT
Comment on attachment 15853 [details]
PAtch for bug

Fix looks fine. Needs a regression test.
Comment 3 David Kilzer (:ddkilzer) 2007-11-06 17:07:03 PST
Reassigning to webkit-unassigned for more visibility.

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

Comment 4 David Kilzer (:ddkilzer) 2007-11-06 18:05:19 PST
(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.

Comment 5 David Kilzer (:ddkilzer) 2007-11-06 19:15:12 PST
(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).

Comment 6 David Kilzer (:ddkilzer) 2007-11-06 20:15:13 PST
Created attachment 17078 [details]
Patch v2

Now with correct fix and 3 layout tests.
Comment 7 Darin Adler 2007-11-07 10:44:38 PST
Comment on attachment 17078 [details]
Patch v2

r=me

(I probably would have used h/v instead of longer names.)
Comment 8 David Kilzer (:ddkilzer) 2007-11-07 12:05:46 PST
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.)

Comment 9 David Kilzer (:ddkilzer) 2007-11-07 13:15:58 PST
(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().

Comment 10 David Kilzer (:ddkilzer) 2007-11-07 13:58:05 PST
Committed revision 27581.

Comment 11 David Kilzer (:ddkilzer) 2007-11-07 14:01:48 PST
(In reply to comment #7)
> (I probably would have used h/v instead of longer names.)

I made this change before committing.