Address VK spec review comments. #317

snianu · 2021-07-23T19:32:46Z

Closes #????

The following tasks have been completed:

Confirmed there are no ReSpec/BikeShed errors or warnings.
Modified Web platform tests (link to pull request)

Implementation commitment:

WebKit (https://bugs.webkit.org/show_bug.cgi?id=)
Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=856269)
Gecko (https://bugzilla.mozilla.org/show_bug.cgi?id=)

snianu · 2021-07-23T21:07:48Z

There are some ReSpec errors related to Reflect, ReflectOnly & DOMRect. Not sure why is it not able to find DOMRect from [geometry]? Maybe @johanneswilm knows?

saschanaz · 2021-07-23T22:18:01Z

There are some ReSpec errors related to Reflect, ReflectOnly & DOMRect. Not sure why is it not able to find DOMRect from [geometry]? Maybe @johanneswilm knows?

Line 43 should be "geometry-1" instead, since that's what specref recognizes. @sidvishnoi, do you know why that difference happens, since https://respec.org/xref shows "geometry" instead?

saschanaz · 2021-07-23T22:21:33Z

(While you are at it, could you please remove line 46 and replace line 6 to refer to respec-w3c instead?)

snianu · 2021-07-23T22:24:49Z

@saschanaz That fixed the DOMRect errors, thank you so much! I'm still getting errors for Reflect & ReflectOnly, do you know how to resolve those?

saschanaz · 2021-07-23T22:36:29Z

Reflect/ReflectOnly have never been defined in any spec, see whatwg/html#3238. I recommend to follow what WICG/attribution-reporting-api#67 says until it resolves.

Instead, imitate https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element:reflect by saying things like "The IDL attribute conversiondestination, impressiondata, ... must reflect the respective content attributes of the same name."

johanneswilm · 2021-07-24T09:28:11Z

docs/virtualkeyboard/index.html

@@ -167,15 +166,24 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
            </dt>
            <dd>
              <p>
-                When this attribute is set to `true`, a user agent MUST NOT resize its layout viewport or visual viewport.
+                A boolean, initially false. When this attribute is set to `true`, a user agent MUST NOT resize its layout viewport or visual viewport.


false => false

sidvishnoi · 2021-07-24T11:09:38Z

@sidvishnoi, do you know why that difference happens, since https://respec.org/xref shows "geometry" instead?

@saschanaz https://respec.org/xref (xref UI) makes use of data from webref, while respec uses specref. Filed speced/respec-web-services#224, though it's probably something that specref needs to update.

yoavweiss

A few comments

yoavweiss · 2021-07-25T12:41:22Z

docs/virtualkeyboard/index.html

@@ -119,7 +118,7 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                  The method must follow these steps:
                </p>
                <ol>
-                    <li>If the browsing context's <a href="https://html.spec.whatwg.org/multipage/browsers.html#active-window">active window</a> does not have <a href="https://html.spec.whatwg.org/multipage/interaction.html#sticky-activation">sticky user activation</a>, abort these steps.
+                    <li>If the <a href="https://heycam.github.io/webidl/#this">this's </a><a href="https://html.spec.whatwg.org/#concept-relevant-global">relevant global object</a> is a <a href="https://html.spec.whatwg.org/multipage/window-object.html#window">Window object</a> and does not have <a href="https://html.spec.whatwg.org/multipage/interaction.html#sticky-activation">sticky user activation</a>, abort these steps.


The links could use cross-references

"the" is not needed before this. The "'s" following this would be better outside the link.

Could this be called from a worker? If not, it's better to put this's relevant global in a variable, assert that it's a Window, and then abort if there's no user activation.

yoavweiss · 2021-07-25T12:49:13Z

docs/virtualkeyboard/index.html

@@ -131,7 +130,7 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                      Call the system API to show the VK.
                  </li>
                  <li>
-                    When the VK is shown by the system, fire {{VirtualKeyboard/ongeometrychange()}} event.
+                    When the VK is shown by the system, the OS reports the bounding rectangle of the keyboard. Calculate the {{VirtualKeyboard/boundingRect}} which is the intersection of the keyboard rectangle reported by the OS and the layout viewport. Fire {{VirtualKeyboard/ongeometrychange()}} event to JS.


It'd be good to define what a "bounding rectangle" is. https://drafts.csswg.org/cssom-view/#dom-element-getboundingclientrect outlines an algorithm that calculates one. It'd be good to have a similar processing model here.

"Calculate..." should call an algorithm that actually does this calculation.

This step should run in parallel

yoavweiss · 2021-07-25T12:50:28Z

docs/virtualkeyboard/index.html

@@ -143,7 +142,7 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                  The method must follow these steps:
                </p>
                <ol>
-                  <li>If the browsing context is not focused, abort these steps.
+                    <li>If the <a href="https://heycam.github.io/webidl/#this">this's </a><a href="https://html.spec.whatwg.org/#concept-relevant-global">relevant global object</a> is a <a href="https://html.spec.whatwg.org/multipage/window-object.html#window">Window object</a> and does not have <a href="https://html.spec.whatwg.org/multipage/interaction.html#sticky-activation">sticky user activation</a>, abort these steps.


Same comments as above. May be useful to pack this in a helper algorithm.

Just couple of algorithm steps so didn't create a helper

yoavweiss · 2021-07-25T12:59:46Z

docs/virtualkeyboard/index.html

+                A boolean, initially false. When this attribute is set to `true`, a user agent MUST NOT resize its layout viewport or visual viewport.
+              </p>
+              <p>
+                The {{VirtualKeyboard/overlaysContent}} getter steps are to return <a href="https://heycam.github.io/webidl/#this">this's </a>{{VirtualKeyboard/overlaysContent}}.


It'd be better to set and get a separate internal boolean variable.

Can you give me an example? I'm following https://xhr.spec.whatwg.org/#dom-xmlhttprequest-timeout

On a second look, this is fine as is. It'd have been slightly better to define the getter as a separate concept, but that's not critical.

w3c#314

snianu · 2021-07-27T01:40:16Z

@yoavweiss Addressed all comments. PTAL.

yoavweiss

A few more comments

yoavweiss · 2021-07-27T11:16:19Z

docs/virtualkeyboard/index.html

+                        Let `lv` be the layout viewport's bounds in CSS pixels.
+                    </li>
+                    <li>
+                        Set [=this=]'s {{VirtualKeyboard/boundingRect}} to be the intersection of `lv` and `osk` in CSS pixels.


This algorithm feels like a getter, rather than a setter. If so, it should return a DOMRect but it doesn't create one. I'd expect it to do so while initializing it to the right values.

Added a DOMRect object to store the intersection of the rectangles.

yoavweiss · 2021-07-27T11:16:39Z

docs/virtualkeyboard/index.html

            </dt>
            <dd>
            <p>
-                The attribute reports the intersection of the VK with the layout viewport in client coordinates.
+                The attribute reports the intersection of the VK with the layout viewport in client coordinates. It is set when {{VirtualKeyboard/ongeometrychange}} event is fired.
+                Setter steps are as follows:


There's no setter for a readonly attribute.

oops, you are right. I'll remove it

yoavweiss · 2021-07-27T11:18:19Z

docs/virtualkeyboard/index.html

-                </li>
+                  </li>
+                  <li>
+                    When the VK is hidden by the system, the OS reports the bounding rectangle of the keyboard (which has all 0 values). Calculate the {{VirtualKeyboard/boundingRect}} which is the intersection of the keyboard rectangle reported by the OS and the layout viewport. Fire {{VirtualKeyboard/ongeometrychange()}} event to JS.


"Calculate ...." should call an algorithm, or simply assign a 0 sized DOMRect (since that's the intersection IIUC)

yoavweiss · 2021-07-27T11:21:00Z

docs/virtualkeyboard/index.html

@@ -131,7 +130,7 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                      Call the system API to show the VK.
                  </li>
                  <li>
-                    When the VK is shown by the system, fire {{VirtualKeyboard/ongeometrychange()}} event.
+                    When the VK is shown by the system, the OS reports the bounding rectangle of the keyboard. Calculate the {{VirtualKeyboard/boundingRect}} which is the intersection of the keyboard rectangle reported by the OS and the layout viewport. Fire {{VirtualKeyboard/ongeometrychange()}} event to JS.


"Calculate..." should call an algorithm that actually does this calculation.

yoavweiss · 2021-07-27T11:22:27Z

docs/virtualkeyboard/index.html

@@ -131,7 +130,7 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                      Call the system API to show the VK.
                  </li>
                  <li>
-                    When the VK is shown by the system, fire {{VirtualKeyboard/ongeometrychange()}} event.
+                    When the VK is shown by the system, the OS reports the bounding rectangle of the keyboard. Calculate the {{VirtualKeyboard/boundingRect}} which is the intersection of the keyboard rectangle reported by the OS and the layout viewport. Fire {{VirtualKeyboard/ongeometrychange()}} event to JS.


This step should run in parallel

yoavweiss · 2021-07-27T11:23:11Z

docs/virtualkeyboard/index.html

@@ -116,7 +126,11 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                  The method must follow these steps:
                </p>
                <ol>
-                    <li>If the browsing context's <a href="https://html.spec.whatwg.org/multipage/browsers.html#active-window">active window</a> does not have <a href="https://html.spec.whatwg.org/multipage/interaction.html#sticky-activation">sticky user activation</a>, abort these steps.
+                  <li>
+                    Let `window` be [=this=]'s <a href="https://html.spec.whatwg.org/#concept-relevant-global">relevant global object</a>. If window is not a <a href="https://html.spec.whatwg.org/multipage/window-object.html#window">Window object</a>, then abort these steps.


the window variable should be referred to as "|window|".

snianu · 2021-07-28T01:24:38Z

@yoavweiss Addressed all comments. PTAL.

yoavweiss

Getting closer... A few more comments

yoavweiss · 2021-07-28T05:57:36Z

docs/virtualkeyboard/index.html

+        </p>
+        <dfn>overlaysContent</dfn>
+        <p>
+            A boolean, initially `false`. When this attribute is set to `true`, a user agent MUST NOT resize its layout viewport or <a href="https://wicg.github.io/visual-viewport/#dom-visualviewport">visual viewport</a>.


IntersectionObserver uses the "document's viewport", pointing to https://drafts.csswg.org/css2/#viewport
That seems more precise than "layout viewport".

yoavweiss · 2021-07-28T06:04:53Z

docs/virtualkeyboard/index.html

@@ -116,7 +126,11 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                  The method must follow these steps:
                </p>
                <ol>
-                    <li>If the browsing context's <a href="https://html.spec.whatwg.org/multipage/browsers.html#active-window">active window</a> does not have <a href="https://html.spec.whatwg.org/multipage/interaction.html#sticky-activation">sticky user activation</a>, abort these steps.
+                  <li>
+                    Let |window| be [=this=]'s <a href="https://html.spec.whatwg.org/#concept-relevant-global">relevant global object</a>. If |window| is not a <a href="https://html.spec.whatwg.org/multipage/window-object.html#window">Window object</a>, then abort these steps.


I think you can replace the abort with an assertion that |window| is a [=Window=] (because this is only exposed on Window)

Nit: You can replace the explicit links (here and elsewhere) with spec references. e.g. "Let |window| be [=this=]'s [=relevant global object=]. Assert that |window| is a {{Window}} object."

yoavweiss · 2021-07-28T06:10:55Z

docs/virtualkeyboard/index.html

+                The attribute reports the intersection of the VK with the layout viewport in client coordinates. It is set when {{VirtualKeyboard/ongeometrychange}} event is fired. To create a {{VirtualKeyboard/boundingRect}} follow these steps:
+                <ol>
+                    <li>
+                        Let `osk` be the on-screen keyboard rectangle in CSS pixels that is provided by the OS when the VK visibility changes.


All references to variables (osk, lv, bounds, etc) should be of the form "|var|" rather than "var". That enables various features such as variable highlighting, and makes it clearer that they are in fact vaariables

Also, |osk| and |lv| should be input parameters, and the caller should provide them

yoavweiss · 2021-07-28T06:13:18Z

docs/virtualkeyboard/index.html

            </dt>
            <dd>
            <p>
-                The attribute reports the intersection of the VK with the layout viewport in client coordinates.
+                The attribute reports the intersection of the VK with the layout viewport in client coordinates. It is set when {{VirtualKeyboard/ongeometrychange}} event is fired. To create a {{VirtualKeyboard/boundingRect}} follow these steps:


No need to mention when it is set here. It'd be sufficient to define an algorithm here (e.g. "set a virtual keyboard bounding rect"), and then call it before the event is fired.

yoavweiss · 2021-07-28T06:20:21Z

docs/virtualkeyboard/index.html

@@ -128,7 +142,7 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                      Call the system API to show the VK.
                  </li>
                  <li>
-                    When the VK is shown by the system, fire {{VirtualKeyboard/ongeometrychange()}} event.
+                    When the VK is shown by the system, <a href="https://html.spec.whatwg.org/#in-parallel">in parallel</a> the OS reports the bounding rectangle of the keyboard. Update the {{VirtualKeyboard/boundingRect}} which is the intersection of the keyboard rectangle reported by the OS and the layout viewport. Fire {{VirtualKeyboard/ongeometrychange()}} event to JS.


You can move the "in parallel" bit up, into something like:
"""

[=In parallel=], follow these steps: * Wait for the virtual keyboard to be shown by the system. * Call "set the virtual keyboard bounding rect" with the keyboard's OS reported bounding rectangle and the [=document=]'s [=viewport=]. * [=Fire an event=] named "geometrychange" at [=this=].

Done. Couldn't use [=viewport=] because for some reason css xref's are not working?

@marcoscaceres - can you help on the [=viewport=] front?

yoavweiss · 2021-07-28T06:20:48Z

docs/virtualkeyboard/index.html

@@ -140,17 +154,21 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
                  The method must follow these steps:
                </p>
                <ol>
-                  <li>If the browsing context is not focused, abort these steps.
+                  <li>
+                    Let |window| be [=this=]'s <a href="https://html.spec.whatwg.org/#concept-relevant-global">relevant global object</a>. If |window| is not a <a href="https://html.spec.whatwg.org/multipage/window-object.html#window">Window object</a>, then abort these steps.


Same comment as above on abort and links

yoavweiss · 2021-07-28T06:21:12Z

docs/virtualkeyboard/index.html

+                    Let |window| be [=this=]'s <a href="https://html.spec.whatwg.org/#concept-relevant-global">relevant global object</a>. If |window| is not a <a href="https://html.spec.whatwg.org/multipage/window-object.html#window">Window object</a>, then abort these steps.
+                  </li>
+                  <li>
+                    If |window| does not have <a href="https://html.spec.whatwg.org/multipage/interaction.html#sticky-activation">sticky user activation</a>, abort these steps.


Same nit on replacing links with references

yoavweiss · 2021-07-28T06:21:47Z

docs/virtualkeyboard/index.html

-                </li>
+                  </li>
+                  <li>
+                    When the VK is hidden by the system, <a href="https://html.spec.whatwg.org/#in-parallel">in parallel</a> the OS reports the bounding rectangle of the keyboard (which has all 0 values). Create a {{DOMRect}} with 0 values and return it when {{VirtualKeyboard/boundingRect}} is queried. Fire {{VirtualKeyboard/ongeometrychange()}} event to JS.


Same comment as above on revamping to multiple steps

yoavweiss · 2021-07-28T06:26:19Z

docs/virtualkeyboard/index.html

+                        Let `bounds` be a {{DOMRect}} object.
+                    </li>
+                    <li>
+                        Set `bounds` to be the intersection of `lv` and `osk` in CSS pixels.


It'd be better to do something like https://www.w3.org/TR/intersection-observer/#calculate-intersection-rect-algo here. You can't really reuse it as is, because |osk| is not a DOM element...

snianu · 2021-07-29T01:59:44Z

@yoavweiss Addressed all comments. PTAL.

yoavweiss

LGTM

I still have a few nits around turning links to refs, and one question on the IDL change, but overall this looks great. Thanks for bearing with me! :)

yoavweiss · 2021-07-29T04:28:58Z

docs/virtualkeyboard/index.html

-                The attribute reports the intersection of the VK with the layout viewport in client coordinates.
+                The attribute reports the intersection of the VK with the [=document=]'s <a href="https://drafts.csswg.org/css2/#viewport">viewport</a> in client coordinates. Call {{VirtualKeyboard/set the virtual keyboard bounding rect}}.
+                <p>
+                    <p><dfn>set the virtual keyboard bounding rect</dfn></p>


"To set the virtual keyboard bounding rect, with |osk| (a DOMRect representing the on-screen keyboard rectangle), and |lv| (a DOMRect representing the document's viewport rectangle) as inputs, run the following steps:"

yoavweiss · 2021-07-29T04:32:52Z

docs/virtualkeyboard/index.html

@@ -189,17 +250,17 @@ <h2>The <dfn>VirtualKeyboard</dfn> Interface</h2>
        <h2>Extensions to the <dfn>ElementContentEditable</dfn> mixin</h2>
        <pre class="idl">
            partial interface ElementContentEditable {
-                [CEReactions, Reflect, ReflectOnly=("auto","manual")] attribute DOMString virtualKeyboardPolicy;
+                [CEReactions] attribute DOMString virtualKeyboardPolicy;


Why remove the IDL reflection rules?

I was getting ReSpec errors so the recommendation (per this comment) is to just mention it in the definition of the attribute and link it to the IDL. In my case I mentioned it in the virtualKeyboardPolicy definition:

The virtualKeyboardPolicy is an enumerated attribute whose keywords are the string, auto, and manual. The IDL attribute must reflect the respective content attributes of the same name.

snianu · 2021-07-29T07:09:16Z

Thank you @yoavweiss for reviewing it!

snianu requested a review from BoCupp-Microsoft July 23, 2021 19:32

snianu mentioned this pull request Jul 23, 2021

VirtualKeyboard spec feedback #314

Closed

snianu changed the title ~~Address spec review comments.~~ Address VK spec review comments. Jul 23, 2021

johanneswilm reviewed Jul 24, 2021

View reviewed changes

johanneswilm approved these changes Jul 24, 2021

View reviewed changes

yoavweiss reviewed Jul 25, 2021

View reviewed changes

snianu added 4 commits July 26, 2021 18:33

Address spec review comments in

8ecb6dd

w3c#314

Fix ReSpec errors.

c39cbf8

Address PR comments.

a2adc47

Rebase and fix errors.

5fb75f6

snianu force-pushed the vk-spec-update branch from 3a10836 to 5fb75f6 Compare July 27, 2021 01:37

yoavweiss reviewed Jul 27, 2021

View reviewed changes

Addressed PR comments.

e57850e

yoavweiss reviewed Jul 28, 2021

View reviewed changes

Address PR comments.

914979f

yoavweiss approved these changes Jul 29, 2021

View reviewed changes

Address PR comments.

2a43726

snianu merged commit 489acb4 into w3c:gh-pages Jul 29, 2021

snianu deleted the vk-spec-update branch July 29, 2021 07:09

Address VK spec review comments. #317

Address VK spec review comments. #317

Conversation

snianu commented Jul 23, 2021 • edited Loading

snianu commented Jul 23, 2021 • edited Loading

saschanaz commented Jul 23, 2021

saschanaz commented Jul 23, 2021

snianu commented Jul 23, 2021

saschanaz commented Jul 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidvishnoi commented Jul 24, 2021 • edited Loading

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snianu commented Jul 27, 2021

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snianu commented Jul 28, 2021

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snianu commented Jul 29, 2021

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snianu commented Jul 29, 2021

snianu commented Jul 23, 2021 •

edited

Loading

snianu commented Jul 23, 2021 •

edited

Loading

sidvishnoi commented Jul 24, 2021 •

edited

Loading