Bug 90532 – Shadow DOM for img element

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

The spec says <img> should behave like having a ShadowRoot.
Currently when we add a Shadow DOM to <img>, it does not have almost no effect.

[Shinya Kawanaka]

Created attachment 150741 [details]
W.I.P.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13125767

[Early Warning System Bot]

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

[Build Bot]

Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13133697

[Early Warning System Bot]

Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13139457

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13139474

[Build Bot]

Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13130778

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alexey Proskuryakov]

> 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.

[Hayato Ito]

(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.

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 150855 [details]
WIP

[Shinya Kawanaka]

I will add more tests if this patch compiles.

[Build Bot]

Comment on attachment 150855 [details]
WIP

Attachment 150855 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13136879

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Shinya Kawanaka]

Created attachment 151170 [details]
Patch

[Hajime Morrita]

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>

[Shinya Kawanaka]

Created attachment 151383 [details]
Patch

[Shinya Kawanaka]

(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.

[Build Bot]

Comment on attachment 151383 [details]
Patch

Attachment 151383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13166435

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 151404 [details]
Patch

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 151410 [details]
Patch

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 151579 [details]
Patch

[Shinya Kawanaka]

(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.

[Build Bot]

Comment on attachment 151579 [details]
Patch

Attachment 151579 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13208182

[Build Bot]

Comment on attachment 151579 [details]
Patch

Attachment 151579 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13200162

[Shinya Kawanaka]

Created attachment 151584 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #37)
> Created an attachment (id=151584) [details]
> Patch

Try to fix mac build.

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 151660 [details]
Patch

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 151664 [details]
Patch

[Shinya Kawanaka]

Created attachment 151883 [details]
Patch

[Shinya Kawanaka]

(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.

[Build Bot]

Comment on attachment 151883 [details]
Patch

Attachment 151883 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13198702

[Shinya Kawanaka]

Created attachment 151913 [details]
Patch

[Shinya Kawanaka]

Created attachment 152107 [details]
Patch

[Ryosuke Niwa]

<img> is a tag. We're not certainly not adding shadow DOM to a HTML tag.

[Ryosuke Niwa]

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/.

[Shinya Kawanaka]

Created attachment 152666 [details]
Patch (Changed ChangeLog)

[Hajime Morrita]

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()

[Shinya Kawanaka]

Created attachment 152694 [details]
Patch for landing

[Build Bot]

Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13270039

[WebKit Review Bot]

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

[Early Warning System Bot]

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

[Build Bot]

Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13282041

[Shinya Kawanaka]

Oops.... I've uploaded a wrong patch...

[Gyuyoung Kim]

Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13274051

[Shinya Kawanaka]

Created attachment 152698 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152698 [details]
Patch for landing

Clearing flags on attachment: 152698

Committed r122824: <http://trac.webkit.org/changeset/122824>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.