Summary: | Shadow DOM for img element | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||||||||||||||||||||||||||||||||||||||
Component: | DOM | Assignee: | Shinya Kawanaka <shinyak> | ||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, cmarcelo, dglazkov, dominicc, eric.carlson, eric, feature-media-reviews, gustavo, gyuyoung.kim, hayato, japhet, jochen, macpherson, menard, mifenton, morrita, philn, rakuco, rniwa, tasak, tkent, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 91167, 97867 | ||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 82313 | ||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Shinya Kawanaka
2012-07-04 02:20:34 PDT
Created attachment 150741 [details]
W.I.P.
Attachment 150741 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/html/shadow/ImageInnerElement.h:31: #ifndef header guard has wrong style, please use: ImageInnerElement_h [build/header_guard] [5]
Source/WebCore/html/shadow/ImageInnerElement.h:46: The parameter name "document" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/html/shadow/ImageInnerElement.h:54: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/html/shadow/ImageInnerElement.cpp:32: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Source/WebCore/html/shadow/ImageInnerElement.cpp:33: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 5 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13125767 Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13130768 Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13133697 Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13139457 Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13131762 New failing tests: fast/dom/shadow/shadowdom-for-image.html Created attachment 150761 [details]
Archive of layout-test-results from gce-cr-linux-07
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13139474 Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13130778 Comment on attachment 150741 [details] W.I.P. Attachment 150741 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13131773 New failing tests: fast/dom/shadow/shadowdom-for-image.html Created attachment 150770 [details]
Archive of layout-test-results from gce-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
> The spec says <img> should behave like having a ShadowRoot.
Why does the spec say so? Given that current state of shadow DOM, matching a spec doesn't sounds like a valid reason to change implementation.
(In reply to comment #13) > > The spec says <img> should behave like having a ShadowRoot. > > Why does the spec say so? Given that current state of shadow DOM, matching a spec doesn't sounds like a valid reason to change implementation. That's important point. We should not impact an img element as long as users do not add a shadow root. It seems shinya tries to preserve the current img elemnt's structure unless users add a shadowRoot explictly. (In reply to comment #14) > (In reply to comment #13) > > > The spec says <img> should behave like having a ShadowRoot. > > > > Why does the spec say so? Given that current state of shadow DOM, matching a spec doesn't sounds like a valid reason to change implementation. > > That's important point. We should not impact an img element as long as users do not add a shadow root. > > It seems shinya tries to preserve the current img elemnt's structure unless users add a shadowRoot explictly. Yes. I'm trying to keep <img> as is if a Shadow DOM is not attached. My implementation idea is: (1) <img> behavior does not change if Shadow DOM is not attached. (2) When attaching Shadow DOM to <img>, we add our Shadow DOM before attaching Shadow DOM created by a user. Created attachment 150855 [details]
WIP
I will add more tests if this patch compiles. Comment on attachment 150855 [details] WIP Attachment 150855 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13136879 Comment on attachment 150855 [details] WIP Attachment 150855 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13137862 New failing tests: fast/dom/shadow/shadowdom-for-image.html Created attachment 150860 [details]
Archive of layout-test-results from gce-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 150855 [details] WIP Attachment 150855 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13140058 New failing tests: fast/dom/shadow/shadowdom-for-image.html Created attachment 150867 [details]
Archive of layout-test-results from gce-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 151170 [details]
Patch
Comment on attachment 151170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151170&action=review Looks a great start! Let's have a few more iteration. > Source/WebCore/dom/ElementShadow.cpp:77 > + shadowHost->willAddAuthorShadowRoot(); I think it's OK to always. invoke this. Also, this can return false and get validateShadowRoot() merged. > Source/WebCore/html/HTMLImageElement.cpp:80 > + RefPtr<ImageInnerElement> innerElement = ImageInnerElement::create(document()); Following logic is typically called and defined createShadowSubtree(). Such a naming convention is also applicable here. > Source/WebCore/html/HTMLImageElement.cpp:134 > + if (shadow()) shadow() is slow. Please don't call it twice here. > Source/WebCore/html/HTMLImageElement.cpp:164 > + return new (arena) RenderBlock(this); I think this should be able to RenderInline. Why not share the following hasContent() path? > Source/WebCore/html/HTMLImageElement.cpp:308 > + innerElement()->setAttribute(heightAttr, String::number(value)); Is it OK to ignore setWidth()? Why is this needed after all? > Source/WebCore/html/shadow/ImageInnerElement.cpp:49 > + ASSERT(shadowAncestorNode()->hasTagName(HTMLNames::imgTag)); Let's define toHTMLImageElement() in HTMLImageELement.h. That is a pattern for this. > Source/WebCore/html/shadow/ImageInnerElement.cpp:62 > + if (renderer() && renderer()->isImage() && !imageLoader()->hasPendingBeforeLoadEvent()) { Please don't make such a copy-n-taste. Let's share the code somehow. dom/StyleElement.h or html/LabelableElement might be a good reference. > Source/WebCore/html/shadow/ImageInnerElement.cpp:76 > +RenderObject* ImageInnerElement::createRenderer(RenderArena* arena, RenderStyle* style) Ditto. Probably you can just invoke host's createRenderer(). > Source/WebCore/loader/ImageLoader.cpp:94 > + , m_usesInnerElement(false) This approach looks like an abstraction leakage. The loader shouldn't know about shadow tree and something like that. One possible idea is to define a "client" interface to the ImageLoader and implement it in HTMLImageElement to hide these detaiis. > LayoutTests/ChangeLog:25 > + Let's add tests for - events - non-default display styles like inline, none, etc. - light children and <content> Created attachment 151383 [details]
Patch
(In reply to comment #24) > (From update of attachment 151170 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151170&action=review > > Looks a great start! Let's have a few more iteration. > > > Source/WebCore/dom/ElementShadow.cpp:77 > > + shadowHost->willAddAuthorShadowRoot(); > > I think it's OK to always. invoke this. > Also, this can return false and get validateShadowRoot() merged. If we always call this, infinite loop will happen because we add a ShadowRoot before a user adds a ShadowRoot... > > > Source/WebCore/html/HTMLImageElement.cpp:80 > > + RefPtr<ImageInnerElement> innerElement = ImageInnerElement::create(document()); > > Following logic is typically called and defined createShadowSubtree(). > Such a naming convention is also applicable here. Done. > > > Source/WebCore/html/HTMLImageElement.cpp:134 > > + if (shadow()) > > shadow() is slow. Please don't call it twice here. Done. > > > Source/WebCore/html/HTMLImageElement.cpp:164 > > + return new (arena) RenderBlock(this); > > I think this should be able to RenderInline. > Why not share the following hasContent() path? Done. > > > Source/WebCore/html/HTMLImageElement.cpp:308 > > + innerElement()->setAttribute(heightAttr, String::number(value)); > > Is it OK to ignore setWidth()? Why is this needed after all? Removed. It's not necessary. > > > Source/WebCore/html/shadow/ImageInnerElement.cpp:49 > > + ASSERT(shadowAncestorNode()->hasTagName(HTMLNames::imgTag)); > > Let's define toHTMLImageElement() in HTMLImageELement.h. > That is a pattern for this. Done. > > > Source/WebCore/html/shadow/ImageInnerElement.cpp:62 > > + if (renderer() && renderer()->isImage() && !imageLoader()->hasPendingBeforeLoadEvent()) { > > Please don't make such a copy-n-taste. Let's share the code somehow. dom/StyleElement.h or html/LabelableElement might be a good reference. We add ImageElement class to share it among HTMLImageElement and ImageInnerElement. It's something like a mix-in pattern. > > > Source/WebCore/html/shadow/ImageInnerElement.cpp:76 > > +RenderObject* ImageInnerElement::createRenderer(RenderArena* arena, RenderStyle* style) > > Ditto. Probably you can just invoke host's createRenderer(). We don't want to call shadow() in ImageInnerElement actually. We shared the rest of code in ImageElement. > > > Source/WebCore/loader/ImageLoader.cpp:94 > > + , m_usesInnerElement(false) > > This approach looks like an abstraction leakage. > The loader shouldn't know about shadow tree and something like that. > > One possible idea is to define a "client" interface to the ImageLoader and > implement it in HTMLImageElement to hide these detaiis. > Yeah... I made a ImageLoaderClient to conceal it. > > LayoutTests/ChangeLog:25 > > + > > Let's add tests for > - events > - non-default display styles like inline, none, etc. > - light children and <content> Done. Comment on attachment 151383 [details] Patch Attachment 151383 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13166435 Comment on attachment 151383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151383&action=review > Source/WebCore/dom/ElementShadow.cpp:59 > +ElementShadow::ElementShadow() Why did you move these? > Source/WebCore/html/HTMLImageElement.cpp:47 > +class ShadowAwareImageLoaderClient : public ImageLoaderClient { I think HTMLImageElement or ImageElement can implement ImageLoaderClient instead of introducing a new class. > Source/WebCore/html/HTMLImageElement.h:39 > + virtual RenderObject* renderer() const = 0; Having virtual renderer() can hide non-virtual Node::renderer(), which is confusing. I'd prefer to have different name. > Source/WebCore/loader/ImageLoader.h:43 > +class ImageLoaderClient { Typical client is a pure virtual class rather than partially implemented one. > Source/WebCore/loader/ImageLoader.h:53 > + virtual Element* element() const { return m_element; } Exposing the element doesn't help encapsulation. It just complicates things. Created attachment 151404 [details]
Patch
(In reply to comment #28) > (From update of attachment 151383 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151383&action=review > > > Source/WebCore/dom/ElementShadow.cpp:59 > > +ElementShadow::ElementShadow() > > Why did you move these? It's just a mistake... Sorry. > > > Source/WebCore/html/HTMLImageElement.cpp:47 > > +class ShadowAwareImageLoaderClient : public ImageLoaderClient { > > I think HTMLImageElement or ImageElement can implement ImageLoaderClient > instead of introducing a new class. I would like to manage the lifecycle of ImageLoaderClient in ImageLoader... So I think it would be better have a new class. > > > Source/WebCore/html/HTMLImageElement.h:39 > > + virtual RenderObject* renderer() const = 0; > > Having virtual renderer() can hide non-virtual Node::renderer(), which is confusing. > I'd prefer to have different name. Done. > > > Source/WebCore/loader/ImageLoader.h:43 > > +class ImageLoaderClient { > > Typical client is a pure virtual class rather than partially implemented one. Done. > > > Source/WebCore/loader/ImageLoader.h:53 > > + virtual Element* element() const { return m_element; } > > Exposing the element doesn't help encapsulation. It just complicates things. I think hiding all element() is too much. What we would like to do here is to distinguish the element we get attributes from and the element we render an image to. So it would be OK to have 2 methods to return such elements. In most cases, they are the same for now. Created attachment 151410 [details]
Patch
Comment on attachment 151410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151410&action=review As we discussed offline, we also need to take care of renderer side where RenderObject::node() is accessed in varioius ways. > Source/WebCore/html/HTMLImageElement.cpp:65 > + return toElement(shadow->oldestShadowRoot()->firstChild()); It would be worth ASSERTing the tag name. > Source/WebCore/html/HTMLImageElement.h:40 > +protected: Let's consider to kill these virtual method to eliminate the vtable. It looks both are used only in setImageIfNecessary(). So we can pass it as arguments instead of defining virtual method. > Source/WebCore/html/HTMLImageElement.h:134 > + ImageLoaderClient* m_imageLoaderClient; It looks we can eliminate this to save object size. > Source/WebCore/html/HTMLImageLoader.cpp:42 > +public: Nit: Should be non-copyable and fast-allocated. Also, client implementation should be on the client side, not loader side. > Source/WebCore/html/HTMLImageLoader.cpp:57 > +HTMLImageLoader::HTMLImageLoader(Element* element) So we should no longer need this constructor. Created attachment 151579 [details]
Patch
(In reply to comment #32) > (From update of attachment 151410 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151410&action=review > > As we discussed offline, we also need to take care of renderer side where > RenderObject::node() is accessed in varioius ways. I added hostImageElement() in RenderImage. It will return host <img> element if node() is an inner image element. And I replaced node() where its tag name is checked as <img>. > > > Source/WebCore/html/HTMLImageElement.cpp:65 > > + return toElement(shadow->oldestShadowRoot()->firstChild()); > > It would be worth ASSERTing the tag name. Done. > > > Source/WebCore/html/HTMLImageElement.h:40 > > +protected: > > Let's consider to kill these virtual method to eliminate the vtable. > It looks both are used only in setImageIfNecessary(). So we can pass it as arguments instead of defining virtual method. It's good idea. Done. > > > Source/WebCore/html/HTMLImageElement.h:134 > > + ImageLoaderClient* m_imageLoaderClient; > > It looks we can eliminate this to save object size. Done. > > > Source/WebCore/html/HTMLImageLoader.cpp:42 > > +public: > > Nit: Should be non-copyable and fast-allocated. Done. > Also, client implementation should be on the client side, not loader side. Since most of the HTMLImageLoader clients are the same, I would like to provide a default implementation of HTMLImageLoaderClient... There is no suitable place to put it, I decided to put it in HTMLImageLoader.h for now. > > > Source/WebCore/html/HTMLImageLoader.cpp:57 > > +HTMLImageLoader::HTMLImageLoader(Element* element) > > So we should no longer need this constructor. Done. Removed. Comment on attachment 151579 [details] Patch Attachment 151579 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13208182 Comment on attachment 151579 [details] Patch Attachment 151579 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13200162 Created attachment 151584 [details]
Patch
(In reply to comment #37) > Created an attachment (id=151584) [details] > Patch Try to fix mac build. Comment on attachment 151584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151584&action=review It looks this doesn't consider <map> element. But we should. > Source/WebCore/svg/SVGImageLoader.h:39 > +class DefaultSVGImageLoaderClient : public ImageLoaderClient { It looks we can hide this to .cpp file and expose only the create function. > LayoutTests/ChangeLog:17 > + Let's have a test with @alt text. Created attachment 151660 [details]
Patch
(In reply to comment #39) > (From update of attachment 151584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151584&action=review > > It looks this doesn't consider <map> element. But we should. I've added a test. > > > Source/WebCore/svg/SVGImageLoader.h:39 > > +class DefaultSVGImageLoaderClient : public ImageLoaderClient { > > It looks we can hide this to .cpp file and expose only the create function. Done. > > > LayoutTests/ChangeLog:17 > > + > > Let's have a test with @alt text. I've added tests including dynamic alt text update. Created attachment 151664 [details]
Patch
Created attachment 151883 [details]
Patch
(In reply to comment #43) > Created an attachment (id=151883) [details] > Patch As shinyak and morrita discussed this offline, I removed the default implementation of ImageLoaderClient, and used mix-in pattern. Comment on attachment 151883 [details] Patch Attachment 151883 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13198702 Created attachment 151913 [details]
Patch
Created attachment 152107 [details]
Patch
<img> is a tag. We're not certainly not adding shadow DOM to a HTML tag. Comment on attachment 152107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152107&action=review This is an interesting approach. Looks very promising. > Source/WebCore/ChangeLog:8 > + This patch adds Shadow DOM support for <img> element. Nit: s/<img>/img/. Created attachment 152666 [details]
Patch (Changed ChangeLog)
Comment on attachment 152666 [details] Patch (Changed ChangeLog) View in context: https://bugs.webkit.org/attachment.cgi?id=152666&action=review Overall looks good. Added some nits. > Source/WebCore/html/HTMLImageElement.h:142 > + if (!node) Nit: this looks strange. You can do this check in toHTMLImageElement() > Source/WebCore/loader/ImageLoaderClient.h:35 > +#include "Element.h" Do we need this? A forward declaration looks sufficient. > Source/WebCore/loader/ImageLoaderClient.h:47 > + virtual void derefElement() = 0; Which element is going to be ref/deref-ed? The name could imply that. > Source/WebCore/rendering/RenderImage.cpp:571 > + if (node()->hasTagName(imgTag)) Could use isHTMLImageElement() Created attachment 152694 [details]
Patch for landing
Comment on attachment 152694 [details] Patch for landing Attachment 152694 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13270039 Comment on attachment 152694 [details] Patch for landing Attachment 152694 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13280044 Comment on attachment 152694 [details] Patch for landing Attachment 152694 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13269039 Comment on attachment 152694 [details] Patch for landing Attachment 152694 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13282041 Oops.... I've uploaded a wrong patch... Comment on attachment 152694 [details] Patch for landing Attachment 152694 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13274051 Created attachment 152698 [details]
Patch for landing
Comment on attachment 152698 [details] Patch for landing Clearing flags on attachment: 152698 Committed r122824: <http://trac.webkit.org/changeset/122824> All reviewed patches have been landed. Closing bug. |