Bug 219287 - [css-values-4] Support small (sv*), large (lv*) and dynamic (dv*) viewport units
Summary: [css-values-4] Support small (sv*), large (lv*) and dynamic (dv*) viewport units
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 231644 232158 232895 233291 237979
  Show dependency treegraph
 
Reported: 2020-11-25 14:01 PST by Bramus
Modified: 2022-04-18 18:21 PDT (History)
24 users (show)

See Also:


Attachments
[Patch] WIP (93.64 KB, patch)
2021-10-05 18:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] WIP (93.70 KB, patch)
2021-10-05 19:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (103.54 KB, patch)
2021-10-07 17:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (105.94 KB, patch)
2021-10-12 20:43 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (106.79 KB, patch)
2021-10-18 15:18 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (86.97 KB, patch)
2021-10-19 18:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (96.54 KB, patch)
2021-10-19 18:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (96.73 KB, patch)
2021-10-20 12:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bramus 2020-11-25 14:01:21 PST
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
Comment 1 Radar WebKit Bug Importer 2020-12-01 10:46:34 PST
<rdar://problem/71857370>
Comment 2 Luke Diggins 2021-08-27 07:33:38 PDT
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
Comment 3 Devin Rousso 2021-10-05 18:38:40 PDT
Created attachment 440318 [details]
[Patch] WIP
Comment 4 Devin Rousso 2021-10-05 19:02:21 PDT
Created attachment 440319 [details]
[Patch] WIP

rebase
Comment 5 Simon Fraser (smfr) 2021-10-05 20:04:38 PDT
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.
Comment 6 Devin Rousso 2021-10-07 16:22:26 PDT
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 :)
Comment 7 Devin Rousso 2021-10-07 17:17:39 PDT
Created attachment 440557 [details]
Patch

address style/organization feedback
Comment 8 Simon Fraser (smfr) 2021-10-12 16:25:52 PDT
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.
Comment 9 Devin Rousso 2021-10-12 16:38:37 PDT
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>
Comment 10 Devin Rousso 2021-10-12 20:43:25 PDT
Created attachment 441036 [details]
Patch

still need to fix that one test :(
Comment 11 Devin Rousso 2021-10-18 15:18:04 PDT
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
Comment 12 Devin Rousso 2021-10-19 18:46:04 PDT
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.
Comment 13 Devin Rousso 2021-10-19 18:49:17 PDT
Created attachment 441834 [details]
Patch

oops forgot new test files
Comment 14 Devin Rousso 2021-10-20 12:44:39 PDT
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`.
Comment 15 Antti Koivisto 2021-10-21 11:02:48 PDT
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.
Comment 16 Devin Rousso 2021-10-21 11:05:42 PDT
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).
Comment 17 EWS 2021-10-21 11:18:09 PDT
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].
Comment 18 Antti Koivisto 2021-11-04 08:40:13 PDT
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.