836329 - Rem Media Query Condition Bug

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[grayghost]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.56 Safari/537.17

Steps to reproduce:

Preface: This worked in previous versions < 18.0.1 but not as of 1/30/13.
Demo of Firefox Bug: http://cdpn.io/AFjDK


Actual results:

The demo's media query condition for the column count should convert to 1 column below 50rem but doesn't as of Firefox 18.0.1. This bug was also reported for webkit but has been resolved as of 1/30/13. It seems Firefox is playing browser ping pong.


Expected results:

The media query should snap the column count to 1 w/conditions < 50 rem. However, this condition does switch to 2 columns as it should for conditions >= 50rem.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like the behavior changed in http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5ecff3e46ed5&tochange=f9acc2e4d4e3

Given that, possibly a regression from bug 804970...

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we get into nsRuleNode::CalcLengthWithInitialFont, called from nsMediaExpression::Matches.  This calls:

434   return CalcLengthWith(aValue, -1, &defaultFont,
435                         nullptr, aPresContext,
436                         true, false, canStoreInRuleTree);

which is passing -1 for aFontSize and true for aUseProvidedRootEmSize.  Which means we end up setting rootFontSize to -1 and then returning "-50" as the value to compare to the actual viewport width, which is clearly broken.

What I don't understand is how this could have possibly worked...

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> So we get into nsRuleNode::CalcLengthWithInitialFont, called from
> nsMediaExpression::Matches.  This calls:
> 
> 434   return CalcLengthWith(aValue, -1, &defaultFont,
> 435                         nullptr, aPresContext,
> 436                         true, false, canStoreInRuleTree);
> 
> which is passing -1 for aFontSize and true for aUseProvidedRootEmSize. 
> Which means we end up setting rootFontSize to -1 and then returning "-50" as
> the value to compare to the actual viewport width, which is clearly broken.
> 
> What I don't understand is how this could have possibly worked...

Probably because the fixup for aFontSize == -1 moved across to after the 'rem' code in https://hg.mozilla.org/releases/mozilla-aurora/rev/1f9ef298ceeb because I didn't want the broader aStyleContext dependency in code where aCanStoreInRuleTree was set to false.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Fix regression handling 'rem' units in media queries.

I confirmed that the added tests fail without the patch and pass with
the patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 712736 [details] [diff] [review]
Fix regression handling 'rem' units in media queries.

r=me

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28e6c1112583

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 712736 [details] [diff] [review]
Fix regression handling 'rem' units in media queries.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 804970
User impact if declined: media queries using 'rem' units produce incorrect results
Testing completed (on m-c, etc.): on m-c (shortly), mochitests in patch
Risk to taking this patch (and alternatives if risky): very low; patch targeted to the specific case that broke
String or UUID changes made by this patch: none

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/28e6c1112583

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 712736 [details] [diff] [review]
Fix regression handling 'rem' units in media queries.

Low risk fix for a regression found in FF18, this is appropriate for landing to Aurora 20.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/359388a5aa75

Comment 11

[Paul Silaghi, QA [:pauly]]

http://cdpn.io/AFjDK
The 2 columns become 1 column when reducing the browser's size.
Verified fixed FF 20.0a2, 21.0a1 (2013-02-17) Win 7

Comment 12

[Paul Silaghi, QA [:pauly]]

But the testcase (http://fvsch.com/code/bugs/rem-mediaquery/) from https://bugs.webkit.org/show_bug.cgi?id=78295 seems to not be completely fixed in FF 20, 21. The background is not changing right when hitting the colored bars.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That testcase is just wrong.  It even says so itself, if you read carefully enough, though it's pretty obfuscated.