Update for Shadow DOM #4

TakayoshiKochi · 2016-06-23T10:58:24Z

See discussion about this change at:
WICG/webcomponents#192

Splits Document.pointerLockElement into Document.pointerLockElement and
ShadowRoot.pointerLockElement and encapsulates shadow tree from
document.

Other than split, replaces document to 'shadow-including document' where
necessary.

Addresses #3.

TakayoshiKochi · 2016-06-23T10:59:07Z

@scheib @domenic @annevk Could you review?

domenic · 2016-06-23T16:31:37Z

index.html

-          <a href="http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.html#in-a-document">in</a>
+          <p>Pointer lock must succeed only if the <a>target</a>'s
+          <a href="https://dom.spec.whatwg.org/#concept-shadow-including-root">shadow-including root</a> is
+          <a href="https://dom.spec.whatwg.org/#in-a-shadow-including-document">in</a>


I think it's not "in", it's just "is"

scheib · 2016-06-23T22:40:35Z

I'll review again after previous comments addressed, but looking good to me so far.

TakayoshiKochi · 2016-06-24T04:33:18Z

Updated the patch.
Mainly addressed reverting not-in "shadow-including document" to "document".

TakayoshiKochi · 2016-06-24T06:12:36Z

@hayatoito FYI

hayatoito · 2016-06-24T06:48:40Z

index.html

@@ -275,12 +275,12 @@
            for a subsequent requestPointerLock.
          </p>

-          <p>If any element (including this one) in the same document
+          <p>If any element (including this one) in the same shadow-including document


"in a shadow-including document" is defined in DOM Standard, but strictly speaking, "in the same shadow-including document" is not defined, I think.

How about "If any element (including this one) who shares the same shadow-including root of the target"?

Assuming the "in the same shadow-including document" PR#273 will be accepted, can I leave this part as is?

The definition is intended to be used in w3c/pointerlock#4.

domenic · 2016-06-24T15:03:55Z

index.html

          is already locked (or pending lock) the pointer
          lock <a>target</a> must be updated to this element and a
          <a>pointerlockchange</a> event sent.</p>

-          <p>If any element in another document is already locked the request
+          <p>If any element in another shadow-including document is already locked the request


This has a similar problem to the above one @hayatoito noted. Although it is probably not a big deal since I think any reader can figure out what it means. But IMO it would be clearer to talk about roots explicitly:

"If any element whose shadow-including root is a different document is already locked, the request must fail..."

TakayoshiKochi · 2016-06-27T04:59:17Z

Updated the patch draft.
Waiting for whatwg/dom#238 to update "in the same shadow-including document" or "connected to the same document".

TakayoshiKochi · 2016-06-28T06:30:53Z

Updated the description for DocumentOrShadowRoot.pointerLockElement, clarifying the cases whether or not the locked target is in the shadow-including descendant of the context object or not. Thus the spec no longer depends on the "retargeting algorithm" defined in the Shadow DOM spec.

Still waiting whatwg/dom#238 to resolve, but I appreciate if you could review the
updated part of pointerLockElement description.

domenic · 2016-06-28T16:47:30Z

index.html

+           <a href="https://dom.spec.whatwg.org/#concept-shadow-including-descendant">shadow-including descendant</a> of the
+           <a href="http://dom.spec.whatwg.org/#context-object">context object</a>,
+	   return the result of the following algorithm:
+	   <dl>


You can reference shadow DOM for this, sorry for confusing you!

If you did want to inline the algorithm, I think it's better to fit with the conventions of this spec, instead of shadow DOM's idiosyncratic CAPITAL LETTER VARIABLES and input/output <dl>.

But just referencing shadow DOM's algorith, like you did before, is fine. I just thought it's something that should move to DOM eventually, so I commented on that.

@hayatoito pointed out in the code review (for Blink) that strictly speaking "retargeting" is originally designed for event retargeting and differs from what activeElement does in that the result should be in the same tree as the context object:

activeElement must be the result of the retargeting algorithm ..., if the result and the context object are in the same tree.

So eventually we have to put an algorithm for activeElement, pointerLockElement, and fullscreenElement which slightly differs from retargeting somewhere (in dom spec).

Fixed CAPITAL LETTER VARIABLES, input/output to make it look more natural in this spec.

TakayoshiKochi · 2016-06-30T09:46:56Z

Updated the patch.
whatwg/dom#238 made me change "in the same shadow-including document"
to "shadow-including root is same".

domenic · 2016-06-30T18:34:29Z

LGTM!

scheib · 2016-06-30T21:15:28Z

Thank you, LGTM but I will wait for @hayatoito to comment before merging.

hayatoito · 2016-07-01T01:23:02Z

index.html

+             <li>If <var>target</var> is in the same tree as the
+               <a href="http://dom.spec.whatwg.org/#context-object">context object</a>,
+               let <var>adjusted-target</var> be <var>target</var></li>
+             <li>Otherwise: let <var>adjusted-target</var> be the shadow host which is a


Hmm. This definition is not equivalent to the definition which the current activeElement is using.
In your definition, there are multiple candidates of shadow hosts which meet the condition. You can not use "the" shadow host here. :(

Could you try to update the condition? I am happy to help you if you need it. :)

I thought that

there should be one shadow host that matches the conditions, so "THE shadow host"

which meets both of the following conditions (AND condition):

"A shadow-including ancestor" of target (because there can be multiple ancestor shadow hosts), and

is "in THE same tree" as the context object (this should filter only one in the same tree)

therefore I can use "THE shadow host" here.
I'll ask some native speaker around.

(talked offline) I understand the condition that can break my assumption :)
I'll update the sentence.

Updated to return the nearest one in the ancestry. PTAL.

hayatoito · 2016-07-01T13:46:06Z

index.html

+             <li>If <var>target</var> is in the same tree as the
+               <a href="http://dom.spec.whatwg.org/#context-object">context object</a>,
+               let <var>adjusted-target</var> be <var>target</var></li>
+             <li>Otherwise: let <var>adjusted-target</var> be the shadow host of <var>target</var> and repeat the following substeps:


Unfortunately, you can not use "shadow host of A" if A is not DocumentFragment. :(
DOM Standard defines "A's host" only if A is a DocumentFragment, I think.

Instead, you might want to use "A's root’s host" (if A's root is a shadow root, of course).

Updated to use "A's root's host" instead of "A's shadow host".

The precondition requires "the target is in the shadow-including descendant of the context object", i.e. if the context's root is not the same as the target's, the target's root must be a shadow root.

TakayoshiKochi · 2016-07-04T06:53:08Z

@haytoito PTAL

hayatoito · 2016-07-04T08:02:10Z

index.html

+         <dd>
+           <dfn title="pointerLockElement"></dfn>
+
+           <p>If the element, which is set as the target for mouse events while the pointer is


I recommend you to use a retargeting algorithm as is so that we do not have a lot of specilized retargeting algorithms.
I am afraid that we will have a maintanace burden.

I think your definition works here because it is for DocumentOrShadowRoot, and it is for Pointer locked element.
If one of these assumption breaks, your definition will not work.
I have to review your definition carefully here...
e.g. The case where the target's root is neither Document nor ShadowRoot. Have you considered this case??

I prefer having one algorithm, and we should refer it from everywhere if we can apply it, instead of customizing it in many places.

Got the point.

Updated to use retargeting algorithm.
PTAL.

TakayoshiKochi · 2016-07-07T02:44:19Z

@hayatoito Ping?

hayatoito · 2016-07-07T03:46:44Z

LGTM.
Let me work on upstreaming retargeting algorithm to DOM Standard , whatwg/dom#276.
After that, we can update Pointer Lock specification, again, so that it does not refer to Shadow DOM spec.

TakayoshiKochi · 2016-07-07T04:37:16Z

@hayatoito Thanks for the review!

@scheib would you mind merging this?

I'll update the reference to the "retargeting algorithm" (from shadow dom spec to DOM spec)
once whatwg/dom#276 is resolved.

scheib · 2016-07-07T17:38:13Z

index.html

+           <a href="https://dom.spec.whatwg.org/#concept-shadow-including-descendant">shadow-including descendant</a> of the
+           <a href="http://dom.spec.whatwg.org/#context-object">context object</a>,
+	   returns the result of the
+           <a href="https://https://w3c.github.io/webcomponents/spec/shadow/#dfn-retargeting-algorithm">retargeting algorithm</a>


double https

Fixed, and other instances of using "http:" instead of "https" also fixed.

TakayoshiKochi · 2016-07-08T07:44:58Z

@hayatoito I updated the algorithm, dropping the condition to check if the target is shadow-including descendant of the context object or not. Without the condition check, the case should be covered by the same tree check of retargeting result and context object. Alternatively, if we keep the shadow-including descendant check, we can drop the same tree check.
I think it is better to drop the shadow-including descendant check to keep in sync with the current activeElement spec for future maintenance cost.

What do you think?

TakayoshiKochi · 2016-07-08T07:50:15Z

I've always used git push -f to update the change here, but just in case you want to see
the revision history, see my "shadowdom2" branch:
gh-pages...TakayoshiKochi:shadowdom2

scheib · 2016-07-08T20:49:02Z

index.html

+                &lt;/div&gt;
+              &lt;/body&gt;
+          </pre>
+          <p>Note: the example uses fictional <code>shadow-root</code> element to denote a


In this section I think that all ids can be used raw, without needing the element type prefixes. E.g.
canvas#canvas1 -> canvas1
div#host1 -> host1

scheib · 2016-07-08T20:56:11Z

Thanks for the example. The simplified algorithm is much easier to understand this way as well. I'll wait for hayatoito, but otherwise LGTM.

hayatoito · 2016-07-11T23:08:37Z

I think it is better to drop the shadow-including descendant check to keep in sync with the current activeElement spec for future maintenance cost.

Yeah, you should drop it. Please be consistent with activeElement. That's what I suggested.

LGTM.

See discussion about this change at: WICG/webcomponents#192 Splits Document.pointerLockElement into Document.pointerLockElement and ShadowRoot.pointerLockElement and encapsulates shadow tree from document. Other than split, replaces document to use Shadow DOM-aware vocabulary like 'shadow-including root' where necessary. Fixes w3c#3.

TakayoshiKochi · 2016-07-12T04:02:58Z

Thanks for all your reviews!

Now I uploaded the final one, fixing #4 (comment) to remove typename where ID exists.

hayatoito · 2016-08-01T03:48:29Z

@TakayoshiKochi

Now you can use https://dom.spec.whatwg.org/#retarget. Could you update the spec again?

TakayoshiKochi · 2016-08-29T10:50:19Z

Sorry for the delay, #8 is the PR for the update.

domenic reviewed Jun 23, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch from 9203577 to 860154b Compare June 24, 2016 04:28

hayatoito reviewed Jun 24, 2016
View reviewed changes

hayatoito added a commit to whatwg/dom that referenced this pull request Jun 24, 2016

Shadow: Define "in the same shadow-including document"

4b2c9c0

The definition is intended to be used in w3c/pointerlock#4.

hayatoito mentioned this pull request Jun 24, 2016

Shadow: Define "in the same shadow-including document" whatwg/dom#273

Closed

TakayoshiKochi force-pushed the shadowdom branch from 860154b to e750369 Compare June 24, 2016 12:03

domenic reviewed Jun 24, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch from e750369 to 4dad448 Compare June 27, 2016 04:58

TakayoshiKochi force-pushed the shadowdom branch from 4dad448 to 3d07201 Compare June 28, 2016 06:25

domenic reviewed Jun 28, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch 2 times, most recently from e23cf49 to aff3835 Compare June 30, 2016 09:44

hayatoito reviewed Jul 1, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch from f6f8888 to 85f660a Compare July 4, 2016 06:50

hayatoito reviewed Jul 4, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch 2 times, most recently from ea50ed7 to 3ea549a Compare July 5, 2016 05:05

annevk mentioned this pull request Jul 5, 2016

Upstream retargeting algorithm whatwg/dom#276

Closed

scheib reviewed Jul 7, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch 3 times, most recently from ca1c754 to 6b20580 Compare July 8, 2016 07:20

scheib reviewed Jul 8, 2016
View reviewed changes

TakayoshiKochi force-pushed the shadowdom branch from a8b6086 to fc87295 Compare July 12, 2016 02:37

scheib merged commit efc77b2 into w3c:gh-pages Jul 12, 2016

TakayoshiKochi mentioned this pull request Aug 29, 2016

Use the new "retargeting algorighm" in the DOM spec. #8

Merged

Update for Shadow DOM #4

Update for Shadow DOM #4

Uh oh!

Conversation

TakayoshiKochi commented Jun 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TakayoshiKochi commented Jun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheib commented Jun 23, 2016

Uh oh!

TakayoshiKochi commented Jun 24, 2016

Uh oh!

TakayoshiKochi commented Jun 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Jun 27, 2016

Uh oh!

TakayoshiKochi commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Jun 30, 2016

Uh oh!

domenic commented Jun 30, 2016

Uh oh!

scheib commented Jun 30, 2016

Uh oh!

hayatoito Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hayatoito Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Jul 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Jul 7, 2016

Uh oh!

hayatoito commented Jul 7, 2016

Uh oh!

TakayoshiKochi commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Jul 8, 2016

Uh oh!

TakayoshiKochi commented Jul 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheib commented Jul 8, 2016

Uh oh!

hayatoito commented Jul 11, 2016

Uh oh!

TakayoshiKochi commented Jul 12, 2016

TakayoshiKochi commented Jun 23, 2016 •

edited

Loading

hayatoito Jul 1, 2016 •

edited

Loading

hayatoito Jul 1, 2016 •

edited

Loading