Bug 156684

<rdar://problem/25778616>

CSS Media Queries conformance test: http://test.csswg.org/harness/test/mediaqueries-3_dev/single/relative-units-001/

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.

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

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.

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.

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.

Here is another testcase in relative units: https://codepen.io/nico3333fr/full/xYMJrg/ (in french, but easily understandable)

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?

(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.

Created attachment 365812 [details] Patch

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.

Up for grabs.

For clarification, this bug is only about the condition of the media query, and not about the body of the media query.

Created attachment 444786 [details] Patch

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?

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

Created attachment 444911 [details] WIP

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.

Created attachment 444912 [details] Patch

Created attachment 444990 [details] Patch for committing

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].

*** Bug 78295 has been marked as a duplicate of this bug. ***