Investigate why ImageDocument uses UA shadow root |
||||
Issue descriptionThis question came up during code review: https://chromium-review.googlesource.com/c/chromium/src/+/798935
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7f9604b49d394f189fde0bf9689cf5c28815a0f commit c7f9604b49d394f189fde0bf9689cf5c28815a0f Author: Takayoshi Kochi <kochi@chromium.org> Date: Wed Jan 24 03:20:19 2018 Remove UA shadow usage from ImageDocument. The usage of UA shadow in ImageDocument was introduced at https://chromium.googlesource.com/chromium/src/+/8512a83031e6531c976f4af85a28dbeef58bd635 but it seems there is no necessity to use shadow dom at all. Bug: 802185 Change-Id: Iab0d9ec36f2f8e5b167d88d73d7e5bbed40599b7 Reviewed-on: https://chromium-review.googlesource.com/877693 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Takayoshi Kochi <kochi@chromium.org> Cr-Commit-Position: refs/heads/master@{#531407} [modify] https://crrev.com/c7f9604b49d394f189fde0bf9689cf5c28815a0f/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-document-default-src-none-expected.txt [modify] https://crrev.com/c7f9604b49d394f189fde0bf9689cf5c28815a0f/third_party/WebKit/Source/core/html/ImageDocument.cpp
,
Jan 24 2018
,
Jan 24 2018
From historical design doc for the new ImageDocument, I found the following passage: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81Ng/edit Getting the mocks above also required doing the following: ... Concerns ... - Any attempt by the page to attach another shadow root would fail According to the thread: Shadow DOM v1 doesn't allow author shadow roots on the body, so from the spec level we're fine. Shadow DOM v0 did, but I doubt intersection of v0 and people messing with image documents is large. We already used the UA shadow root strategy for video IIRC. And this was a shortened summary of shadow-related discussion at blink-dev: https://groups.google.com/a/chromium.org/d/topic/blink-dev/6SKGZNeUj0k/discussion Elliott Sprehn: Note that the shape of the document body is actually required per spec, but we can use a UA shadow root here. Philip Jägenstedt Even if UA shadow roots are used, I think that'd be observable in that any attempt by the page to attach another shadow root would then fail. Pretty obscure, but I think it'd be worth investigating if these documents actually need to be considered same-origin with the page that embeds them. Elliott Sprehn: Shadow DOM v1 doesn't allow author shadow roots on the body, so from the spec level we're fine. Shadow DOM v0 did, but I doubt the intersection of v0 and people messing with image documents is large. We already used the UA shadow root strategy for video IIRC.
,
Jan 24 2018
Probably Elliott suggested that attaching <image> directly under <body> doesn't work well for sizing the image and background, but Shadow DOM can be used for resizing container. But Philip's first comment can be interpreted as "UA shadow root as a container *to hide* the image from external doesn't work because it's observable", which doesn't correspond to what Elliott meant. The image document and image are same origin or not is irrelevant to whether we use UA shadow or not. The last comment from Elliott means <video> is using UA shadow, but the comment is confusing because MeidaDocument is not using UA shadow at all for its body. From above, I'd conclude that usage of UA shadow root in ImageDocument is coming from these confused discussion on blink-dev, and usage of UA shadow in ImageDocument is unnecessary.
,
Jan 24 2018
> ote that the shape of the document body is actually required per spec, but we can use a UA shadow root here. Hmm, so, now we violates the HTML standard. https://html.spec.whatwg.org/multipage/browsing-the-web.html#read-media > initialize the Document object, append an html element to the Document, append a head element and a body element to the html element, append an element host element for the media, as described below, to the body element, ... IMO, we should have kept the UA shadow unless we have a strong reason against it.
,
Jan 24 2018
> > ote that the shape of the document body is actually required per spec, but we can use a UA shadow root here. > > Hmm, so, now we violates the HTML standard. Could you elabolate in which point we are violating the HTML standard?
,
Jan 24 2018
> Could you elabolate in which point we are violating the HTML standard? We have a DIV between BODY and IMG if ShouldShrinkToFit(). According to the HTML standard, DIV must not exist. This document structure is observable with IFRAME with image resource and content scripts injecting image documents.
,
Jan 24 2018
I see, I'll revert the change and add some comment about this. Thanks!
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4decd94510c3ddd502c939edd8dc5a04ceb9658f commit 4decd94510c3ddd502c939edd8dc5a04ceb9658f Author: Takayoshi Kochi <kochi@chromium.org> Date: Wed Jan 24 10:32:36 2018 Revert "Remove UA shadow usage from ImageDocument." Reverts "Remove UA shadow usage from ImageDocument." https://chromium-review.googlesource.com/877693 After more investigation, the removal turned out violating the HTML spec. The corresponding HTML spec: https://html.spec.whatwg.org/multipage/browsing-the-web.html#read-media mandates <body> for the ImageDocument must have only one <img> element. The previous change introduced intermediate container <div> element, but it is not conforming to the spec. Originally the UA shadow was attached to <body> and hides the container and this CL restores the behavior. Added a WPT test for checking the case. There was an existing test case for this, but when ImageDocument is embedded in an <iframe>, the new media-like view isn't enabled and no UA shadow is used. The new test creates a popup window which is a shrinkable container, and Blink's UA shadow implementation is used. Other UA implementation may benefit from this (or may not, but at least do not do harm). Bug: 802185 Change-Id: Ifac013eced8f1d234092acffb726a90c03f988f9 Reviewed-on: https://chromium-review.googlesource.com/882630 Commit-Queue: Takayoshi Kochi <kochi@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Cr-Commit-Position: refs/heads/master@{#531494} [add] https://crrev.com/4decd94510c3ddd502c939edd8dc5a04ceb9658f/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/read-media/pageload-image-in-popup.html [modify] https://crrev.com/4decd94510c3ddd502c939edd8dc5a04ceb9658f/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-document-default-src-none-expected.txt [modify] https://crrev.com/4decd94510c3ddd502c939edd8dc5a04ceb9658f/third_party/WebKit/Source/core/html/ImageDocument.cpp
,
Jan 26 2018
,
Jan 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/657576bc067c891f21c7b2b567d97812325b8e3e commit 657576bc067c891f21c7b2b567d97812325b8e3e Author: Takayoshi Kochi <kochi@chromium.org> Date: Mon Jan 29 03:08:56 2018 Add comment about why ImageDocument uses UA shadow DOM. Bug: 802185 Change-Id: Ibbffe82ac4dfdeb200e608deb908dbbc691459f1 Reviewed-on: https://chromium-review.googlesource.com/888459 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Takayoshi Kochi <kochi@chromium.org> Cr-Commit-Position: refs/heads/master@{#532311} [modify] https://crrev.com/657576bc067c891f21c7b2b567d97812325b8e3e/third_party/WebKit/Source/core/html/ImageDocument.cpp |
||||
►
Sign in to add a comment |
||||
Comment 1 by kochi@chromium.org
, Jan 23 2018