New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 803274 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 806159



Sign in to add a comment

Have GuestViewContainer use Shadow DOM v1

Project Member Reported by mcnee@chromium.org, Jan 18 2018

Issue description

GuestViewContainer creates a shadow DOM in which the internal element is placed. We're currently calling |createShadowRoot| on the container element, which means we're using Shadow DOM v0. We should use |attachShadow| for Shadow DOM v1 instead.
 

Comment 1 by mcnee@chromium.org, Jan 18 2018

Another thing to consider is whether to make the resulting shadow DOM open or closed. The current v0 shadow dom is implicitly open.

From previous discussions ( crbug.com/178663  ,  crbug.com/229205 ), it would seem like we would want to make it closed.

Comment 2 by lfg@chromium.org, Jan 24 2018

Cc: kochi@chromium.org
Components: Blink>DOM>ShadowDOM
+kochi who's looking at deprecating Shadow DOM v0.

Comment 3 by kochi@chromium.org, Jan 26 2018

Blocking: 806159

Comment 4 by kochi@chromium.org, Apr 19 2018

Is this still true?

Comment 6 by kochi@google.com, Apr 23 2018

Thanks,

I haven't figured out the implication of using shadow dom in guest view
container, so I should be missing something.

Tentatively changing createShadowRoot to attachShadow to see what breaks at:
https://chromium-review.googlesource.com/c/chromium/src/+/1023677

Comment 7 by kochi@google.com, Apr 23 2018

Looks like the following tests failed.
WebViewTests/WebViewAccessibilityTest.FocusAccessibility/1
WebViewTests/WebViewAccessibilityTest.FocusAccessibility/0
WebViewInteractiveTests/WebViewFocusInteractiveTest.Focus_AdvanceFocus/1
WebViewInteractiveTests/WebViewFocusInteractiveTest.FocusAndVisibility/0
WebViewInteractiveTests/WebViewFocusInteractiveTest.FocusAndVisibility/1

Comment 8 Deleted

Comment 9 by kochi@chromium.org, May 15 2018

It seems the guest view container looks like using Custom Elements V0 as well.
https://cs.chromium.org/chromium/src/extensions/renderer/resources/guest_view/guest_view_container.js?l=209
It has to be migrated to Custom Elements v1 API as well.

Does anyone in the webview team has any plan working on this?

Comment 10 by mcnee@chromium.org, May 15 2018

Labels: -Pri-3 Pri-2
Owner: mcnee@chromium.org
Status: Assigned (was: Available)
Re c#9: Once I finish with the Site Isolation input issues I'm currently working on, I can take a look at this.

Are there any resources describing the differences in behaviour between the v0 and v1 versions of Shadow DOM and Custom Elements? From c#7, it looks like there is a difference in how focus is handled between Shadow DOM v0 and v1.

Comment 11 by kochi@chromium.org, May 31 2018

Sorry not revisiting this for a while.
https://hayato.io/2016/shadowdomv1/ would be a good resource to start with
for migration from Shadow DOM v0 to v1.  I'm not sure the real reason for
the failures in #c7, maybe something was not rendered.

Components: -Blink>DOM>ShadowDOM
Adding to this, for eventually removing all Shadow DOM V0 and Custom Elements
V0 infrastructure from Blink, <webview> needs to migrate from Custom Elements V0
to Custom Elements V1.
Filed  issue 867831  for comment13.
Status: Started (was: Assigned)
So there appears to be a behaviour change between Shadow DOM v0 and v1 regarding tabindex. Currently the element to which we attach the shadow dom has tabindex=-1. The contents of the frame in the shadow dom are reachable with v0, but they're unreachable with v1. If I remove the tabindex, keyboard focus works properly with v1.

Fortunately, it looks like the tabindex on the container is no longer necessary.
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4caaa081a11650f79f34786d5a8ff1c923ed626f

commit 4caaa081a11650f79f34786d5a8ff1c923ed626f
Author: Kevin McNee <mcnee@chromium.org>
Date: Fri Aug 10 19:44:49 2018

Remove tabindex from the GuestViewContainer element.

A tabindex of -1 on the GuestViewContainer element allows it to be
focusable but not reachable via sequential keyboard navigation.

There does not appear to be any further need for the GuestViewContainer
element itself to be focusable given that we override the focus method.
Previously, we had a focus event listener on the GuestViewContainer
element in order to propagate the focus to the internal element which
would explain why this was needed.

See
https://crrev.com/6476573b7d9099b04002736d90a078550c8651eb
https://crrev.com/53ba951bb6ed4df0998240520fed9d00a7ffff67

Bug:  231664 ,  803274 
Change-Id: If6ffb6f7282db86a1a7ab9e856785e6f51d79a80
Reviewed-on: https://chromium-review.googlesource.com/1170988
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582294}
[modify] https://crrev.com/4caaa081a11650f79f34786d5a8ff1c923ed626f/extensions/renderer/resources/guest_view/guest_view_container.js

Project Member

Comment 17 by bugdroid1@chromium.org, Aug 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e61fa07cd555ca35a730605c75725f6db5bb8698

commit e61fa07cd555ca35a730605c75725f6db5bb8698
Author: Kevin McNee <mcnee@chromium.org>
Date: Mon Aug 13 17:06:10 2018

Migrate GuestViewContainer to Shadow DOM v1

Bug:  803274 
Change-Id: If56ee6ad0220b429b4497ccecef90eb257265478
Reviewed-on: https://chromium-review.googlesource.com/1170989
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582621}
[modify] https://crrev.com/e61fa07cd555ca35a730605c75725f6db5bb8698/extensions/renderer/resources/guest_view/guest_view_container.js

Status: Fixed (was: Started)
So now that we're using v1, does anyone have any thoughts about whether we should make the guest view container's shadow root closed? It seems like the only benefit would be indicating our intent that guest view elements' shadow roots should be treated like user agent shadow roots.

As mentioned in the above migration doc, it doesn't offer any guarantees since Element.prototype.attachShadow could be clobbered by user scripts.
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/058930e68573f2dd66725d0145277af89f35a576

commit 058930e68573f2dd66725d0145277af89f35a576
Author: Kevin McNee <mcnee@chromium.org>
Date: Thu Nov 29 16:59:07 2018

Close the GuestView shadow root

Now that we're using Shadow DOM v1, we have the option to make the
shadow DOM unreachable by outside script by specifying a closed
encapsulation mode. Now GuestViews will behave more like other
elements defined by the browser which internally use shadow DOM.
This also prevents script from interfering with our internal elements
and producing unexpected behaviour.

Bug: 892886,  803274 
Change-Id: I57b2513e3ff7290e286b3d8391e7d93bac2bc4e9
Reviewed-on: https://chromium-review.googlesource.com/c/1342768
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612232}
[modify] https://crrev.com/058930e68573f2dd66725d0145277af89f35a576/chrome/test/data/extensions/platform_apps/web_view/shim/main.js
[modify] https://crrev.com/058930e68573f2dd66725d0145277af89f35a576/extensions/browser/guest_view/web_view/web_view_apitest.cc
[modify] https://crrev.com/058930e68573f2dd66725d0145277af89f35a576/extensions/renderer/resources/guest_view/guest_view_container.js
[modify] https://crrev.com/058930e68573f2dd66725d0145277af89f35a576/extensions/test/data/web_view/apitest/main.js

Sign in to add a comment