Bug 232895 - [css-values-4] Support `*vi` (inline) and `*vb` (block) viewport units
Summary: [css-values-4] Support `*vi` (inline) and `*vb` (block) 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
: 159801 (view as bug list)
Depends on: 219287
Blocks: 234373
  Show dependency treegraph
 
Reported: 2021-11-09 11:14 PST by Devin Rousso
Modified: 2022-07-05 14:05 PDT (History)
12 users (show)

See Also:


Attachments
Patch (48.36 KB, patch)
2021-11-09 11:19 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (49.19 KB, patch)
2021-11-11 12:04 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (49.15 KB, patch)
2021-11-16 14:42 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (48.38 KB, patch)
2021-11-16 15:45 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (70.94 KB, patch)
2021-12-01 14:49 PST, 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 Devin Rousso 2021-11-09 11:14:35 PST
This is part of css-values-4 <https://drafts.csswg.org/css-values-4/#viewport-variants>.

`*vi` is equal to 1% of the size of the UA-default/large/small/dynamic viewport size in the direction of the root element’s inline axis.

`*vb` is equal to 1% of the size of the initial containing block UA-default/small/large/dynamic viewport size in the direction of the root element’s block axis.

The implementation will likely take the form of looking at the direction of the root element's block/inline axis and using that to grab the corresponding direction from the `v*`/`sv*`/`lv*`/`dv*` unit (the latter three of which were implemented in bug 219287).

As an example, on `about:blank`:
- `vi` would be equal to `vw`
- `svi` would be equal to `svw`
- `lvi` would be equal to `lvw`
- `dvi` would be equal to `dvw`
- `vb` would be equal to `vh`
- `svb` would be equal to `svh`
- `lvb` would be equal to `lvh`
- `dvb` would be equal to `dvh`
Comment 1 Devin Rousso 2021-11-09 11:14:53 PST
<rdar://problem/85179134>
Comment 2 Devin Rousso 2021-11-09 11:19:53 PST
Created attachment 443709 [details]
Patch
Comment 3 Devin Rousso 2021-11-11 12:04:17 PST
Created attachment 443986 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-11-15 15:00:00 PST
Comment on attachment 443986 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:669
> +static double lengthOfPhysicalAxisForLogicalAxisOfViewport(LogicalBoxAxis logicalAxis, const FloatSize& size, const RenderStyle* rootElementStyle)

does the "of viewport" refer to the logical axis or the length?
Comment 5 Devin Rousso 2021-11-15 15:07:39 PST
Comment on attachment 443986 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.cpp:669
>> +static double lengthOfPhysicalAxisForLogicalAxisOfViewport(LogicalBoxAxis logicalAxis, const FloatSize& size, const RenderStyle* rootElementStyle)
> 
> does the "of viewport" refer to the logical axis or the length?

it's referring to the axis, i.e. "derive the length of the physical axis of the viewport from the given logical axis of the viewport"

i agree it's a bit of a jumble/mouthfull, so alternative name suggestions are welcome :)
Comment 6 Simon Fraser (smfr) 2021-11-15 20:53:21 PST
Comment on attachment 443986 [details]
Patch

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

>>> Source/WebCore/css/CSSPrimitiveValue.cpp:669
>>> +static double lengthOfPhysicalAxisForLogicalAxisOfViewport(LogicalBoxAxis logicalAxis, const FloatSize& size, const RenderStyle* rootElementStyle)
>> 
>> does the "of viewport" refer to the logical axis or the length?
> 
> it's referring to the axis, i.e. "derive the length of the physical axis of the viewport from the given logical axis of the viewport"
> 
> i agree it's a bit of a jumble/mouthfull, so alternative name suggestions are welcome :)

lengthOfPhysicalAxisForViewportLogicalAxis()
or
lengthOfViewportPhysicalAxisForLogicalAxis()
Comment 7 Devin Rousso 2021-11-16 14:42:00 PST
Created attachment 444432 [details]
Patch
Comment 8 Devin Rousso 2021-11-16 15:45:08 PST
Created attachment 444446 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-11-30 14:15:49 PST
Comment on attachment 444446 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:695
> +    const auto* rootElement = renderView.document().documentElement();
> +    if (!rootElement)
> +        return 0;

This makes me think we should test these units in an SVG document (i.e. main document is SVG).

> Tools/ChangeLog:3
> +        [css-values-4] Support `*vi` (inline) and `*vb` (block) viewport units

Can we write some WPT?
Comment 10 Devin Rousso 2021-11-30 16:17:37 PST
Comment on attachment 444446 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.cpp:695
>> +        return 0;
> 
> This makes me think we should test these units in an SVG document (i.e. main document is SVG).

This appears to work just fine on things like <https://webkit.org/wp-content/themes/webkit/images/webkit.svg>.  I can add another API test for a scenario like this.

>> Tools/ChangeLog:3
>> +        [css-values-4] Support `*vi` (inline) and `*vb` (block) viewport units
> 
> Can we write some WPT?

My plan was to write some WPT once all of the new viewport units were implemented.  Also, given how much viewport units rely on the browser defining the viewport (as well as any changes in the meaning thereof during the lifetime of the page) I think they'd likely just end up being parsing (and maybe basic validation that they match `width: 100%` and `height: 100%`) and I frankly didn't feel like it would be a good use of my time compared to an API test when I was implementing this (due to all the other stuff I had on my plate).  My load is quite lighter now so I'll look into this very soon.
Comment 11 Jen Simmons 2021-11-30 16:29:17 PST
>My plan was to write some WPT once all of the new viewport units were implemented.  Also, given how much viewport units rely on the browser defining the viewport (as well as any changes in the meaning thereof during the lifetime of the page) I think they'd likely just end up being parsing (and maybe basic validation that they match `width: 100%` and `height: 100%`) and I frankly didn't feel like it would be a good use of my time compared to an API test when I was implementing this (due to all the other stuff I had on my plate).  My load is quite lighter now so I'll look into this very soon.

We want to include the new Viewport Units (sv*, dv*, lv*, and *vi, *vb) in the Interop 2022 project. So there is outside scrutiny looking to see the state of testing right now. 

We don't need the tests to be done *now*, but we do need a way to advocate for inclusion of the new Viewport units, and not have them rejected because of a lack of tests. Perhaps you can summarize your thoughts & plans for these tests at https://github.com/web-platform-tests/interop-2022/issues/4. We are meeting this week (Thursday) to review status.
Comment 12 Devin Rousso 2021-11-30 16:48:42 PST
Comment on attachment 444446 [details]
Patch

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

>>> Source/WebCore/css/CSSPrimitiveValue.cpp:695
>>> +        return 0;
>> 
>> This makes me think we should test these units in an SVG document (i.e. main document is SVG).
> 
> This appears to work just fine on things like <https://webkit.org/wp-content/themes/webkit/images/webkit.svg>.  I can add another API test for a scenario like this.

I should clarify that I added `height: 100vb` and `height: 100vi` using Web Inspector to the root `<svg>`.  That SVG document doesn't use any of the units added in this patch by default.
Comment 13 Devin Rousso 2021-12-01 14:49:19 PST
Created attachment 445619 [details]
Patch
Comment 14 EWS 2021-12-02 15:26:31 PST
Committed r286458 (244799@main): <https://commits.webkit.org/244799@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445619 [details].
Comment 15 Sam Sneddon [:gsnedders] 2022-07-05 14:05:05 PDT
*** Bug 159801 has been marked as a duplicate of this bug. ***