Bug 156684

Summary: rem in media queries should be calculated using font-size:initial, not root element font-size
Product: WebKit Reporter: Chris Rebert <webkit>
Component: CSSAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: danburzo, dante3333, david.humphrey, eric.carlson, esprehn+autocc, ews-watchlist, florens, glenn, gyuyoung.kim, jer.noble, koivisto, macpherson, menard, m.goleb+bugzilla, mmaxfield, philipj, rbuis, rik, sergio, webkit-bug-importer, xously
Priority: P2 Keywords: HasReduction, InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://plnkr.co/edit/x7Ix0qUw0K5YVT4TDN5h?p=preview
See Also: https://bugs.webkit.org/show_bug.cgi?id=156687
https://bugs.webkit.org/show_bug.cgi?id=194749
Bug Depends on:    
Bug Blocks: 159753    
Attachments:
Description Flags
Testcase for rem media-queries bug
none
Patch
none
Patch
none
WIP
none
Patch
koivisto: review+
Patch for committing none

Description Chris Rebert 2016-04-17 18:08:39 PDT
Per https://drafts.csswg.org/mediaqueries/#units
> Relative units in media queries are based on the initial value,
> which means that units are never based on results of declarations.
> For example, in HTML, the 'em' unit is relative to the initial value of 'font-size',
> defined by the user agent or the user’s preferences, not any styling on the page.

The term "initial value" refers to https://drafts.csswg.org/css-cascade/#initial-value
and "relative units" includes "Relative length units" (https://drafts.csswg.org/css-values/#relative-lengths ),
which includes 'em' and 'rem'.

Per https://drafts.csswg.org/css-fonts/#propdef-font-size ,
the initial value of the 'font-size' property is 'medium',
which major web browsers (including WebKit) compute to 16px under default settings.

WebKit currently appears to take author style declarations into account when computing 'rem'-based widths in viewport media queries,
in violation of the aforementioned section of the Media Queries spec.

Testcase demonstrating the bug: https://plnkr.co/edit/x7Ix0qUw0K5YVT4TDN5h?p=preview

Chrome, Firefox, and IE11 comply with said section of the spec.
See also http://zellwk.com/blog/media-query-units/#1-font-size-changed-in-html
Comment 1 Radar WebKit Bug Importer 2016-04-18 08:38:03 PDT
<rdar://problem/25778616>
Comment 2 Chris Rebert 2016-12-15 01:17:47 PST
CSS Media Queries conformance test:
http://test.csswg.org/harness/test/mediaqueries-3_dev/single/relative-units-001/
Comment 3 David Humphrey (humphd) 2017-12-07 08:23:55 PST
I spent some time looking at this, and was able to get the attached plnkr test case to pass with a hacky "fix".

// WebCore/css/CSSPrimitiveValue.cpp
double CSSPrimitiveValue::computeNonCalcLengthDouble(const CSSToLengthConversionData& conversionData, UnitType primitiveType, double value)
{
    double factor;
    bool applyZoom = true;

    switch (primitiveType) {
    ...
    case CSS_REMS:
        // hack to force use of initial font vs. user specified root style
        auto fontDescription = conversionData.style()->fontDescription();
        fontDescription.setKeywordSizeFromIdentifier(CSSValueMedium);
        factor = fontDescription.specifiedSize();
        break;

I realize this isn't the right way to do it, but I was curious to try and track it down.  If someone with proper knowledge of the css code is willing to guide me a bit on a proper fix, and assuming it doesn't require massive changes I'm not equipped to make, I'd be willing to take this further.

Thanks for any guidance.
Comment 4 Myles C. Maxfield 2018-01-02 17:07:06 PST
Thanks for the detailed analysis and investigation! I think you're on the right track, but we probably wouldn't have to create a whole FontDescription just to set the size correctly. I'd instead call Style::fontSizeForKeyword(). If we can do that, I don't think it would be a hack; I think it would be fine to commit.

Are you interested in doing this, or should I pick it up from you?

Thanks again!
Myles
Comment 5 David Humphrey (humphd) 2018-01-07 13:56:25 PST
Thanks for your comment, Myles.  I'm glad that I wasn't way off.  Yes, I think I'll try and finish this, if that's alright.  I'll follow-up with any questions I have based on what you've written above.
Comment 6 David Humphrey (humphd) 2018-01-20 11:20:05 PST
Myles, I went to finish this, and had a question. I realize now why I did things the way I did vs. using Style::fontSizeForKeyword() as you suggest--I couldn't figure out a clean way to get a document reference in CSSPrimitiveValue.cpp via the conversionData that I have, such that I can pass it in as the third arg.  I've spent a bunch of time trying to find a way to do it, but I'm stumped.  Could you give me some insight as to how to approach it?  Thanks for any tips.  I'm enjoying learning things as I go, but running into the limits of my knowledge.
Comment 7 Nicolas H. 2018-03-01 06:38:33 PST
Created attachment 334815 [details]
Testcase for rem media-queries bug

1. Adjust your browser window's width above 961px.
2. Go to https://hteumeuleu.fr/wp-content/uploads/2018/03/fx-media-queries-rem.html (or attached file).
3. Resize the browser window's width below 960px.
Comment 8 Nicolas H. 2018-03-01 06:39:25 PST
Here is another testcase in relative units: https://codepen.io/nico3333fr/full/xYMJrg/ (in french, but easily understandable)
Comment 9 Myles C. Maxfield 2018-03-01 13:28:48 PST
Hang on, computeNonCalcLengthDouble() is used for all style resolution, not just media queries. This is a situation where, if a media-query says "60rem" it refers to a different value than if an element says "60rem".

So we can't just modify computeNonCalcLengthDouble(). We have to set some state inside CSSToLengthConversionData which represents whether or not we are trying to calculate a media query or a regular style on an element.

Style::fontSizeForKeyword() is sensitive to the user's preferences, namely settings().defaultFixedFontSize() and settings().defaultFontSize(). Media queries probably should be sensitive to these preferences as well, which means we should use the logic inside Style::fontSizeForKeyword().

You're right about not having access to a document inside CSSPrimitiveValue, and this is desirable - individual CSS values shouldn't know what a whole Document is. Luckily, the CSSToLengthConversionData is created in MediaQueryEvaluator::evaluate() which has access to m_document. So we should interrogate m_document inside MediaQueryEvaluator::evaluate() and use our findings to set some new fields (which we will create) inside CSSToLengthConversionData. Then, computeNonCalcLengthDouble() can read those settings.

We should make sure that when we test this, we test all the relevant combinations of defaultFontSize(), defaultFixedFontSize(), media queries, and style on regular elements.

Does that help you understand this?
Comment 10 Myles C. Maxfield 2018-03-01 13:30:31 PST
(In reply to Myles C. Maxfield from comment #9)
> Hang on, computeNonCalcLengthDouble() is used for all style resolution, not
> just media queries. This is a situation where, if a media-query says "60rem"
> it refers to a different value than if an element says "60rem".
> 
> So we can't just modify computeNonCalcLengthDouble(). We have to set some
> state inside CSSToLengthConversionData which represents whether or not we
> are trying to calculate a media query or a regular style on an element.
> 
> Style::fontSizeForKeyword() is sensitive to the user's preferences, namely
> settings().defaultFixedFontSize() and settings().defaultFontSize(). Media
> queries probably should be sensitive to these preferences as well, which
> means we should use the logic inside Style::fontSizeForKeyword().
> 
> You're right about not having access to a document inside CSSPrimitiveValue,
> and this is desirable - individual CSS values shouldn't know what a whole
> Document is. Luckily, the CSSToLengthConversionData is created in
> MediaQueryEvaluator::evaluate() which has access to m_document. So we should
> interrogate m_document inside MediaQueryEvaluator::evaluate() and use our
> findings to set some new fields (which we will create) inside
> CSSToLengthConversionData. Then, computeNonCalcLengthDouble() can read those
> settings.
> 
> We should make sure that when we test this, we test all the relevant
> combinations of defaultFontSize(), defaultFixedFontSize(), media queries,
> and style on regular elements.
> 
> Does that help you understand this?

Oh, and we're going to have to refactor Style::fontSizeForKeyword() to make a version that doesn't accept a whole Document, but just accepts the things that it would have asked the document for.
Comment 11 Rob Buis 2019-03-23 06:17:30 PDT
Created attachment 365812 [details]
Patch
Comment 12 Dan Burzo 2019-09-26 02:01:12 PDT
Not sure this is a helpful addition at this point, but currently, because `rem`s in media queries relate to the html font-size rather than the initial font-size, you can create infinite loops:

@media (min-width: 60rem) {
  html {
    font-size: 20rem;
  }
}

This manifests in Safari 12 macOS as a continuous toggle in font-size while resizing the window.
Comment 13 Rob Buis 2020-06-08 10:42:41 PDT
Up for grabs.
Comment 14 Myles C. Maxfield 2021-11-18 22:42:30 PST
For clarification, this bug is only about the condition of the media query, and not about the body of the media query.
Comment 15 Myles C. Maxfield 2021-11-19 00:42:58 PST
Created attachment 444786 [details]
Patch
Comment 16 Antti Koivisto 2021-11-20 00:43:46 PST
Comment on attachment 444786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444786&action=review

> Source/WebCore/css/CSSToLengthConversionData.h:102
> +    std::optional<float> m_referenceFontSizeOverride;

It seems awkward that this is an optional "override". Why not just have m_referenceFontSize that is always correct and used consistently?
Comment 17 Myles C. Maxfield 2021-11-20 16:16:01 PST
It looks like this is causing these WPTs to progress:

imported/w3c/web-platform-tests/css/mediaqueries/relative-units-001.html
imported/w3c/web-platform-tests/css/mediaqueries/mq-calc-005.html
Comment 18 Myles C. Maxfield 2021-11-20 16:51:47 PST
Created attachment 444911 [details]
WIP
Comment 19 Myles C. Maxfield 2021-11-20 17:01:40 PST
Comment on attachment 444911 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=444911&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:654
> +    if (referenceFontSizeOverride)
> +        return *referenceFontSizeOverride;

Hang on, I don't think this is right. If the content says

@media (min-width: 2ex)

that "ex" unit doesn't correspond to the font size; it corresponds to the ex-height of the font which would have been used on the root element if it wasn't styled.
Comment 20 Myles C. Maxfield 2021-11-20 17:54:37 PST
Created attachment 444912 [details]
Patch
Comment 21 Myles C. Maxfield 2021-11-22 16:41:20 PST
Created attachment 444990 [details]
Patch for committing
Comment 22 EWS 2021-11-22 17:46:22 PST
Committed r286123 (244510@main): <https://commits.webkit.org/244510@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444990 [details].
Comment 23 Anthony Ricaud 2022-02-08 09:10:25 PST
*** Bug 78295 has been marked as a duplicate of this bug. ***