use ShadowDOM v1 #49

valdrinkoshi · 2017-03-23T20:04:59Z

Implementation of #48, supporting only native ShadowDOM v1.
Test page with inert and blockingElements - works on Chrome and Safari 🎉 http://jsbin.com/hohegoj/6/edit?html,output

We override HTMLElement.prototype.focus which will do nothing if the node is inert or contained in an inert parent node.

InertManager now keeps track of all inerted nodes: it observes mutations on the light dom, and InertRoot just provides to the manager a way to observe shadowRoot content.

Like this, the manager has visibility on the whole document, even in the shadows!

Tests are 💚. We should figure what strategy to take in polyfilled land.

valdrinkoshi · 2017-04-14T23:18:23Z

@alice @robdodson synced with master, and finally tests are green everywhere! 💪 💚
Mind taking a look?

alice

Apologies for taking a million years to get to this! This looks very promising but I don't fully understand it - see comments inline.

alice · 2017-05-23T04:49:11Z

src/inert.js

+   * has an `inert` attribute.
+   *
+   * Its main functions are:
+   *


Since these points are now just a single line, we can probably get rid of the extra blank lines in between.

alice · 2017-05-23T04:56:08Z

src/inert.js

+      // https://html.spec.whatwg.org/multipage/scripting.html#valid-custom-element-name
+      if (!matches(rootElement, acceptsShadowRootSelector)) {
+        const potentialCustomElement = rootElement.tagName.indexOf('-') !== -1;
+        if (!potentialCustomElement) return;


Can we stick with Chromium style and put the return on a new line?
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style

alice · 2017-05-23T04:57:12Z

src/inert.js

+      // We already failed inerting this shadow root.
+      if (rootElement.__failedAttachShadow) return;
+
+      // Force shadowRoot.


I don't quite understand this comment, could you flesh it out a little?

alice · 2017-05-23T05:03:57Z

src/inert.js

+
+      // Force shadowRoot.
+      if (!rootElement.shadowRoot) {
+        // Detect if this is a closed shadowRoot with try/catch (sigh).


Can we scope the try/catch more tightly? e.g.

let shadowRoot = null; try { shadowRoot = rootElement.attachShadow({mode: 'open'}); } catch (e) { // Most likely a closed shadowRoot was already attached. rootElement.__failedAttachShadow = true; console.warn('Could not inert element shadowRoot', rootElement, e); return; } shadowRoot.appendChild(document.createElement('slot')); rootElement.attachShadow = function() { const slot = this.shadowRoot.querySelector('slot'); slot && this.shadowRoot.removeChild(slot); delete this.attachShadow; return this.shadowRoot; };

alice · 2017-05-23T05:04:09Z

src/inert.js

+          // for polyfilling inert. We ensure the shadowRoot is empty and return it.
+          rootElement.attachShadow = function() {
+            const slot = this.shadowRoot.querySelector('slot');
+            slot && this.shadowRoot.removeChild(slot);


Is there some reason not to just use an if statement here?

alice · 2017-05-23T05:07:18Z

src/inert.js

        }
      }
+      if (typeof onShadowRootMutation === 'function') {


Any reason not to just explicitly check != null?

alice · 2017-05-23T05:13:57Z

src/inert.js

+       * @type {MutationObserver}
+       */
+      this._observer = new MutationObserver(this.watchForInert);
+


mysterious extra line

alice · 2017-05-23T05:14:53Z

src/inert.js

+       */
+      this._inertRoots = new Map();
+
+      this.watchForInert = this._watchForInert.bind(this);


Can we make this "private"? Maybe call it something like this._boundWatchForInert?

alice · 2017-05-23T05:23:14Z

test/index.js

@@ -127,9 +124,9 @@ describe('Basic', function() {
      });
    });

-    describe('ShadowDOM v0', function() {


So we simply won't support v0 at all any more?

Yep, that was the simplistic idea explored by this PR 😁

Does it support browsers which don't support shadow DOM at all?

In the current form, no. We'd have to adopt the previous technique of searching focusable nodes inside an inert node.

To be more precise: this case is currently not supported by this PR:

<div inert> <input> </div>

^ the input will still be reachable via TAB

alice · 2017-05-23T05:33:36Z

src/inert.js

+  HTMLElement.prototype.focus = function() {
+    // If it is inert or into an inert node, no focus!
+    let target = this;
+    while (target && !target.inert) {


So this checks for inertness when focus is attempted? If we're tabbing through a document, and traverse into an inert subtree, how do we know to skip over it and look for the next focusable element?

This override is meant for when focus is forced programmatically (e.g. myElement.focus()), not via user interaction.
On user interactions, there is no situation where the focus would move to an inert node, as we set tabindex=-1 to them. ShadowDom behavior guarantees that these nodes and their content are removed from the tab order.
Did you find any case where this happens on user interactions?

Ohhh I see. No, that makes sense.

valdrinkoshi

Addressed the feedback in the last commit 👌

valdrinkoshi · 2017-05-23T23:59:25Z

src/inert.js

+          return;
+        }
+      } else {
+        // Ensure inerted elements in the shadowRoot have the property updated.


It might be that rootElement had inert children in its shadowRoot and this is the first time we see them, hence we have to update their inert property.

valdrinkoshi · 2017-05-24T00:11:01Z

src/inert.js

+  HTMLElement.prototype.focus = function() {
+    // If it is inert or into an inert node, no focus!
+    let target = this;
+    while (target && !target.inert) {


This override is meant for when focus is forced programmatically (e.g. myElement.focus()), not via user interaction.
On user interactions, there is no situation where the focus would move to an inert node, as we set tabindex=-1 to them. ShadowDom behavior guarantees that these nodes and their content are removed from the tab order.
Did you find any case where this happens on user interactions?

valdrinkoshi · 2017-05-24T00:12:03Z

test/index.js

@@ -127,9 +124,9 @@ describe('Basic', function() {
      });
    });

-    describe('ShadowDOM v0', function() {


Yep, that was the simplistic idea explored by this PR 😁

valdrinkoshi

@alice this PR was made to see how it would look like if we could rely solely on native ShadowDOM.
To support the browsers without native ShadowDOM, we can use the old technique. Would it make sense to handle both cases in the same file? I was thinking to ship a separate file inert-sd.js for browsers with native ShadowDOM in order to deliver a smaller payload. WDYT?

valdrinkoshi · 2017-05-24T17:27:53Z

test/index.js

@@ -127,9 +124,9 @@ describe('Basic', function() {
      });
    });

-    describe('ShadowDOM v0', function() {


In the current form, no. We'd have to adopt the previous technique of searching focusable nodes inside an inert node.

valdrinkoshi added 4 commits March 23, 2017 13:02

use ShadowDOM v1

9144374

add tests for attachShadow called by user

fe5f456

correct check

47de058

break at first found shadowRoot

3af8b0f

valdrinkoshi requested review from alice and robdodson March 23, 2017 21:08

valdrinkoshi added 6 commits March 23, 2017 14:13

global bool for nativeShadowDOM

3892fe1

blur activeElement contained in shadowRoot

99021ff

add styles to shadowRoots. Fix tests

7b27ab2

remove getInertRoot

23d45f9

Merge branch 'master' into sd-v1

4610eb1

lint and build

cef6d70

valdrinkoshi added 2 commits April 17, 2017 10:57

simpler override of focus

5d4ef5c

InertRoot exposes only callback. Handle closed shadowRoots.

ef76b64

alice reviewed May 23, 2017

View reviewed changes

address feedback

ae3302b

valdrinkoshi commented May 24, 2017

View reviewed changes

valdrinkoshi commented May 25, 2017

View reviewed changes

yoavweiss changed the base branch from master to main March 5, 2021 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use ShadowDOM v1 #49

use ShadowDOM v1 #49

valdrinkoshi commented Mar 23, 2017 •

edited

Loading

valdrinkoshi commented Apr 14, 2017

alice left a comment

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

alice May 23, 2017

valdrinkoshi May 24, 2017

alice May 24, 2017

valdrinkoshi May 24, 2017

valdrinkoshi May 25, 2017

alice May 23, 2017

valdrinkoshi May 24, 2017

alice May 24, 2017

valdrinkoshi left a comment

valdrinkoshi May 23, 2017

valdrinkoshi May 24, 2017

valdrinkoshi May 24, 2017

valdrinkoshi left a comment

valdrinkoshi May 24, 2017

use ShadowDOM v1 #49

Are you sure you want to change the base?

use ShadowDOM v1 #49

Conversation

valdrinkoshi commented Mar 23, 2017 • edited Loading

valdrinkoshi commented Apr 14, 2017

alice 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

valdrinkoshi 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

valdrinkoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valdrinkoshi commented Mar 23, 2017 •

edited

Loading