Bug 219287 – [css-values-4] Support small (sv*), large (lv*) and dynamic (dv*) viewport units

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

[Bramus]

See https://github.com/w3c/csswg-drafts/issues/4329 for discussion (specifically https://github.com/w3c/csswg-drafts/issues/4329#issuecomment-577169195)

> The CSS Working Group just discussed New viewport unit, and agreed to the following:
> - RESOLVED: Add a set of viewport units (vhc for ex.) that reflect the size of the layout viewport less all UA UI

Spec text is being discussed in https://github.com/w3c/csswg-drafts/pull/5108

Related issues from other browsers:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1093055
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1610815

[Radar WebKit Bug Importer]

<rdar://problem/71857370>

[Luke Diggins]

vhc has been renamed and expanded as the small (sv*), large (lv*) and dynamic (dv*) viewport units in CSS Values 4: https://drafts.csswg.org/css-values-4/#viewport-variants

[Devin Rousso]

Created attachment 440318 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 440319 [details]
[Patch] WIP

rebase

[Simon Fraser (smfr)]

Comment on attachment 440319 [details]
[Patch] WIP

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

> Source/WebCore/ChangeLog:14
> +        Tests: CSSViewportUnits.AllSame
> +               CSSViewportUnits.EmptyUnobscuredSizeOverrides
> +               CSSViewportUnits.SameUnobscuredSizeOverrides
> +               CSSViewportUnits.DifferentUnobscuredSizeOverrides

I think we need to add some wpt tests that at least test parsing of the new unit types, and minimally test that the "small" variants are smaller than or equal do the "large" variants.

> Source/WebCore/css/CSSPrimitiveValue.h:129
> +    bool isSmallViewportPercentageWidth() const { return primitiveUnitType() == CSSUnitType::CSS_SVW; }
> +    bool isSmallViewportPercentageHeight() const { return primitiveUnitType() == CSSUnitType::CSS_SVH; }
> +    bool isSmallViewportPercentageMax() const { return primitiveUnitType() == CSSUnitType::CSS_SVMAX; }
> +    bool isSmallViewportPercentageMin() const { return primitiveUnitType() == CSSUnitType::CSS_SVMIN; }
> +    bool isLargeViewportPercentageWidth() const { return primitiveUnitType() == CSSUnitType::CSS_LVW; }
> +    bool isLargeViewportPercentageHeight() const { return primitiveUnitType() == CSSUnitType::CSS_LVH; }
> +    bool isLargeViewportPercentageMax() const { return primitiveUnitType() == CSSUnitType::CSS_LVMAX; }
> +    bool isLargeViewportPercentageMin() const { return primitiveUnitType() == CSSUnitType::CSS_LVMIN; }
> +    bool isDynamicViewportPercentageWidth() const { return primitiveUnitType() == CSSUnitType::CSS_DVW; }
> +    bool isDynamicViewportPercentageHeight() const { return primitiveUnitType() == CSSUnitType::CSS_DVH; }
> +    bool isDynamicViewportPercentageMax() const { return primitiveUnitType() == CSSUnitType::CSS_DVMAX; }
> +    bool isDynamicViewportPercentageMin() const { return primitiveUnitType() == CSSUnitType::CSS_DVMIN; }

Are these used anywhere?

> Source/WebCore/css/CSSPrimitiveValue.h:345
>      return type == CSSUnitType::CSS_VW
>          || type == CSSUnitType::CSS_VH
>          || type == CSSUnitType::CSS_VMIN
> -        || type == CSSUnitType::CSS_VMAX;
> +        || type == CSSUnitType::CSS_VMAX
> +        || type == CSSUnitType::CSS_SVW
> +        || type == CSSUnitType::CSS_SVH
> +        || type == CSSUnitType::CSS_SVMIN
> +        || type == CSSUnitType::CSS_SVMAX
> +        || type == CSSUnitType::CSS_LVW
> +        || type == CSSUnitType::CSS_LVH
> +        || type == CSSUnitType::CSS_LVMIN
> +        || type == CSSUnitType::CSS_LVMAX
> +        || type == CSSUnitType::CSS_DVW
> +        || type == CSSUnitType::CSS_DVH
> +        || type == CSSUnitType::CSS_DVMIN
> +        || type == CSSUnitType::CSS_DVMAX;

Maybe this can be a range check.

> Source/WebCore/css/CSSToLengthConversionData.cpp:112
> +double CSSToLengthConversionData::smallViewportWidthFactor() const
> +{
> +    if (m_viewportDependencyDetectionStyle)
> +        m_viewportDependencyDetectionStyle->setHasViewportUnits();
> +
> +    if (!m_renderView)
> +        return 0;
> +
> +    return m_renderView->minimumViewportSizeForCSSSmallViewportUnits().width() / 100.0;
> +}
> +
> +double CSSToLengthConversionData::smallViewportHeightFactor() const
> +{
> +    if (m_viewportDependencyDetectionStyle)
> +        m_viewportDependencyDetectionStyle->setHasViewportUnits();
> +
> +    if (!m_renderView)
> +        return 0;
> +
> +    return m_renderView->minimumViewportSizeForCSSSmallViewportUnits().height() / 100.0;
> +}

This is all getting a bit out of control.

> Source/WebCore/css/CSSToLengthConversionData.h:92
> +    double smallViewportWidthFactor() const;
> +    double smallViewportHeightFactor() const;
> +    double smallViewportMinFactor() const;
> +    double smallViewportMaxFactor() const;
> +    double largeViewportWidthFactor() const;
> +    double largeViewportHeightFactor() const;
> +    double largeViewportMinFactor() const;
> +    double largeViewportMaxFactor() const;
> +    double dynamicViewportWidthFactor() const;
> +    double dynamicViewportHeightFactor() const;
> +    double dynamicViewportMinFactor() const;
> +    double dynamicViewportMaxFactor() const;

Maybe we can halve the number of functions by returning FloatSizes, like 

FloatSize smallViewportFactor() const.

Do they have to be doubles?

> Source/WebCore/css/parser/CSSParserToken.cpp:243
> +        case 'd':
> +            if (toASCIILower(data[1]) == 'v' && toASCIILower(data[2]) == 'm') {
> +                switch (toASCIILower(data[3])) {
> +                case 'a':
> +                    if (toASCIILower(data[4]) == 'x')

This kind of parsing code feels a bit silly.

> Source/WebCore/page/FrameView.h:240
>      void clearViewportSizeOverrideForCSSViewportUnits();
>      IntSize viewportSizeForCSSViewportUnits() const;
>  
> +    WEBCORE_EXPORT void setMinimumViewportSizeForCSSSmallViewportUnits(IntSize);
> +    void clearMinimumViewportSizeOverrideForCSSSmallViewportUnits();
> +    IntSize minimumViewportSizeForCSSSmallViewportUnits() const;
> +
> +    IntSize maximumViewportSizeForCSSLargeViewportUnits() const;
> +    IntSize currentViewportSizeForCSSDynamicViewportUnits() const;

I feel like we should put all the viewport sizes into a struct and just have one function that passes it in.

"Minimum for small" hurts my brain. Do we ever have "maximum for small"? If not, just call it sizeForSmallViewportUnits.

> Source/WebCore/rendering/RenderView.h:149
>      IntSize viewportSizeForCSSViewportUnits() const;
> +    IntSize minimumViewportSizeForCSSSmallViewportUnits() const;
> +    IntSize maximumViewportSizeForCSSLargeViewportUnits() const;
> +    IntSize currentViewportSizeForCSSDynamicViewportUnits() const;

Maybe a single function that returns the one struct with them all in.

[Devin Rousso]

Comment on attachment 440319 [details]
[Patch] WIP

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

>> Source/WebCore/css/CSSPrimitiveValue.h:129
>> +    bool isDynamicViewportPercentageMin() const { return primitiveUnitType() == CSSUnitType::CSS_DVMIN; }
> 
> Are these used anywhere?

Not that I know of, no.  I was following the other methods above.  I'll remove those too while im at it :)

>> Source/WebCore/css/CSSToLengthConversionData.h:92
>> +    double dynamicViewportMaxFactor() const;
> 
> Maybe we can halve the number of functions by returning FloatSizes, like 
> 
> FloatSize smallViewportFactor() const.
> 
> Do they have to be doubles?

Good idea.  I'll switch to those.

I don't know for sure, but given that the implementation does a `/ 100.0f` I think it's a good idea to use `FloatSize` just in case.

>> Source/WebCore/css/parser/CSSParserToken.cpp:243
>> +                    if (toASCIILower(data[4]) == 'x')
> 
> This kind of parsing code feels a bit silly.

In what way?  I was mirroring what the surrounding code did.  How would you do it differently?

>> Source/WebCore/page/FrameView.h:240
>> +    IntSize currentViewportSizeForCSSDynamicViewportUnits() const;
> 
> I feel like we should put all the viewport sizes into a struct and just have one function that passes it in.
> 
> "Minimum for small" hurts my brain. Do we ever have "maximum for small"? If not, just call it sizeForSmallViewportUnits.

I like the name `sizeForSmallViewportUnits`.  I'll rename the existing one while im at it too :)

[Devin Rousso]

Created attachment 440557 [details]
Patch

address style/organization feedback

[Simon Fraser (smfr)]

Comment on attachment 440557 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:706
> +            return std::max(size.width(), size.height()) / 100.0 * value;

If we added FloatSize::maxDimension() const { return std::max(width(), height() } we could do this in one line.

> Source/WebCore/css/CSSPrimitiveValue.cpp:712
> +            return std::min(size.width(), size.height()) / 100.0 * value;

Likewise.

> Source/WebCore/css/CSSPrimitiveValue.cpp:800
> +        return value * std::fmax(defaultViewportFactor.width(), defaultViewportFactor.height());

not sure why you need fmax. Maybe max<float> if needed.

> Source/WebCore/page/FrameView.h:240
> +    WEBCORE_EXPORT void setSizeForCSSDefaultViewportUnits(IntSize);
> +    void clearSizeOverrideForCSSDefaultViewportUnits();
> +    IntSize sizeForCSSDefaultViewportUnits() const;
> +
> +    WEBCORE_EXPORT void setSizeForCSSSmallViewportUnits(IntSize);
> +    void clearSizeOverrideForCSSSmallViewportUnits();
> +    IntSize sizeForCSSSmallViewportUnits() const;
> +
> +    IntSize sizeForCSSLargeViewportUnits() const;
> +    IntSize sizeForCSSDynamicViewportUnits() const;

It's wrong for these to be integral. Views can be fractional pixels wide.

[Devin Rousso]

Comment on attachment 440557 [details]
Patch

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

>> Source/WebCore/page/FrameView.h:240
>> +    IntSize sizeForCSSDynamicViewportUnits() const;
> 
> It's wrong for these to be integral. Views can be fractional pixels wide.

filed a followup <https://webkit.org/b/231644>

[Devin Rousso]

Created attachment 441036 [details]
Patch

still need to fix that one test :(

[Devin Rousso]

Created attachment 441643 [details]
Patch

instead of checking the exact value of `dvw`/`dvh`/`dvmin`/`dvmax`, check that they match `window.innerWidth`/`window.innerHeight` since the value for those units are only partially derived from `-[WKWebView _overrideLayoutParametersWithMinimumLayoutSize:minimumUnobscuredSizeOverride:maximumUnobscuredSizeOverride:]` (i.e. they can also be influenced/adjusted by safe area insets, etc.) and ultimately the goal is to have a way to get the current dimensions of the viewport from CSS

[Devin Rousso]

Created attachment 441833 [details]
Patch

It's actually not necessary to adjust the SPI as the value provided is something akin to a "minimum unobscured size".  The main difference is that the value (`minimumLayoutSize`) is also updated for dynamic viewport size changes, which "small viewport" `svw`/`svh`/`svmin`/`svmax` doesn't really want.  Adjust the changed code to not involve new SPI.

[Devin Rousso]

Created attachment 441834 [details]
Patch

oops forgot new test files

[Devin Rousso]

Created attachment 441918 [details]
Patch

It looks like the `didChangeStyleSheetEnvironment` I added inside `FrameView::unobscuredContentSizeChanged` is causing `http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html` to fail.  That call is needed so that when the current viewport size changes we correctly/automatically update the "dynamic viewport" `dvw`/`dvh`/`dvmin`/`dvmax`.  After doing some digging, we don't actually have to do a full `didChangeStyleSheetEnvironment` as all we really care about is updating any elements that use viewport units, so instead just call `updateViewportUnitsOnResize`.

[Antti Koivisto]

Comment on attachment 441918 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:5624
> +    if (auto* document = frame().document())
> +        document->styleScope().didChangeStyleSheetEnvironment();

When is this stuff being called? didChangeStyleSheetEnvironment can be extremely expensive as it throws out everything we know about style and starts from scratch. It should only ever be used on things like user settings changing, not during any normal operation.

[Devin Rousso]

Comment on attachment 441918 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:5624
>> +        document->styleScope().didChangeStyleSheetEnvironment();
> 
> When is this stuff being called? didChangeStyleSheetEnvironment can be extremely expensive as it throws out everything we know about style and starts from scratch. It should only ever be used on things like user settings changing, not during any normal operation.

This method should only ever be invoked when the actual size of the viewport changes (e.g. `-[WKWebView _overrideLayoutParametersWithMinimumLayoutSize:maximumUnobscuredSizeOverride:]`, auto-size, etc.).

Also FWIW this is actually just a renaming of existing code.  The diff makes it seem like this is new, but in reality `overrideSizeForCSSLargeViewportUnits` is just a rename of `overrideViewportSizeForCSSViewportUnits`.  It's `overrideSizeForCSSSmallViewportUnits` that's new, and that should only ever be called by `-[WKWebView _overrideLayoutParametersWithMinimumLayoutSize:maximumUnobscuredSizeOverride:]` (or if the `WKWebView` is autosized).

[EWS]

Committed r284628 (243349@main): <https://commits.webkit.org/243349@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441918 [details].

[Antti Koivisto]

Comment on attachment 441918 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:5646
> +    // Use the large size if no override is given since it's considered the default size.
> +    // if (m_largeViewportSizeOverride) {
> +    //     if (!viewportSize.width)
> +    //         viewportSize.width = m_largeViewportSizeOverride->width;
> +
> +    //     if (!viewportSize.height)
> +    //         viewportSize.height = m_largeViewportSizeOverride->height;
> +    // }

What is going on here? We don't land commented out code.