Enable author shadows for SVG elements

Enable author shadows for SVG elements

This patch enables author shadows for SVG elements. The referenced bug
no longer applies (SVG's tref was removed) and the shadow code has matured
since. Tests have been added for <g>, <rect>, and <svg> elements.

BUG=416217
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183441

[pdr., 2014-09-20 21:40:28]

pdr@chromium.org changed reviewers:
+ esprehn@chromium.org, shinyak@chromium.org

[esprehn, 2014-09-20 21:57:07]

Can you add a test for <use>? Also what happens if you project content by
using <content> and <shadow>? Create them with createElement and then
appendChild them, since the parser don't.

Need a lot of tests. We might want to control this with a flag so if you
cause a security bug you can disable it in beta and fix for the next
version.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pdr., 2014-09-21 03:49:17]

On 2014/09/20 21:57:07, esprehn wrote:
> Can you add a test for <use>? Also what happens if you project content by
> using <content> and <shadow>? Create them with createElement and then
> appendChild them, since the parser don't.
> 
> Need a lot of tests. We might want to control this with a flag so if you
> cause a security bug you can disable it in beta and fix for the next
> version.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

I'm less excited about this now. I had forgotten it's not yet possible to use
either <shadow> or <content> in SVG due to XMLisms so we're blocked on SVG2 spec
work. This is filed as https://www.w3.org/Bugs/Public/show_bug.cgi?id=24181 and
SVG2 issue 47.

The namespace issue limits the usefulness of this patch so we're limited to
basic custom elements.

[Erik Dahlström (inactive), 2014-09-22 07:57:03]

On 2014/09/21 03:49:17, pdr wrote:
> On 2014/09/20 21:57:07, esprehn wrote:
> > Can you add a test for <use>? Also what happens if you project content by
> > using <content> and <shadow>? Create them with createElement and then
> > appendChild them, since the parser don't.
> > 
> > Need a lot of tests. We might want to control this with a flag so if you
> > cause a security bug you can disable it in beta and fix for the next
> > version.
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> I'm less excited about this now. I had forgotten it's not yet possible to use
> either <shadow> or <content> in SVG due to XMLisms so we're blocked on SVG2
spec
> work. This is filed as https://www.w3.org/Bugs/Public/show_bug.cgi?id=24181
and
> SVG2 issue 47.
> 
> The namespace issue limits the usefulness of this patch so we're limited to
> basic custom elements.

It's true that the namespace/parser issue(s) prevents this from being
super-useful right now, and I agree that it would be good to have a flag for
toggling this. I still think it's a change in the right direction.

Do you have a link to the SVG2 issue?

[pdr., 2014-09-22 20:45:05]

Yeah, I think we should go forward with this patch even if it's not as cool as
it could be.

On 2014/09/22 07:57:03, Erik Dahlström wrote:
> It's true that the namespace/parser issue(s) prevents this from being
> super-useful right now, and I agree that it would be good to have a flag for
> toggling this. I still think it's a change in the right direction.

Because of how limited this is, I'm not too worried about needing a flag to
disable this. I'll sign up for dealing with the fallout if there is any.
Reverting this patch will be very easy since it's only a few lines.
 
> Do you have a link to the SVG2 issue?

I don't think we have a linkable bug for this one (or I am not sure where it
is). It's issue #47 on
https://svgwg.org/svg2-draft/single-page.html#struct-UseElement with the title
"We should define the behavior of ‘use’ in terms of Web Components."

[pdr., 2014-09-24 00:19:20]

Ping?

[pdr., 2014-10-06 06:17:17]

(I asked esprehn not to look at this last week as we had some blocking reviews
that took priority)

@esprehn, could you take a look at this sometime this week?

[Erik Dahlström (inactive), 2014-10-06 10:45:02]

Somewhat related, shouldn't it be possible to also select and style the <use>
shadow trees?

E.g something like this: http://jsfiddle.net/yjd1pxcq/

[dglazkov, 2014-10-06 16:58:23]

lgtm

[pdr., 2014-10-09 02:14:49]

On 2014/10/06 at 10:45:02, ed wrote:
> Somewhat related, shouldn't it be possible to also select and style the <use>
shadow trees?
> 
> E.g something like this: http://jsfiddle.net/yjd1pxcq/

Erik, that's a good question. The shadow tree that <use> creates has a bit set
so it's considered a "user agent shadow root" which hides the fact that there's
a shadow tree down there. There's no reason why we couldn't switch this, it
would just require some spec work. My understanding is that Dirk still has a
todo to rewrite the <use> section in terms of web components.

[pdr., 2014-10-09 02:15:47]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-09 02:17:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/585183004/40001

[commit-bot: I haz the power, 2014-10-09 02:17:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 02:17:59]

Failed to apply patch for Source/core/svg/SVGElement.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/svg/SVGElement.h
  Hunk #1 FAILED at 228.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/svg/SVGElement.h.rej

Patch:       Source/core/svg/SVGElement.h
Index: Source/core/svg/SVGElement.h
diff --git a/Source/core/svg/SVGElement.h b/Source/core/svg/SVGElement.h
index
366065c2bfa103efcfd3e892face544af6fa2bf7..bf4f55455ea320888a892d27ad21e2a2360261e8
100644
--- a/Source/core/svg/SVGElement.h
+++ b/Source/core/svg/SVGElement.h
@@ -228,10 +228,6 @@ protected:
     bool hasFocusEventListeners() const;
 
 private:
-    // FIXME: Author shadows should be allowed
-    // https://bugs.webkit.org/show_bug.cgi?id=77938
-    virtual bool areAuthorShadowsAllowed() const OVERRIDE FINAL { return false;
}
-
     bool isSVGElement() const WTF_DELETED_FUNCTION; // This will catch anyone
doing an unnecessary check.
     bool isStyledElement() const WTF_DELETED_FUNCTION; // This will catch
anyone doing an unnecessary check.

[pdr., 2014-10-09 02:25:53]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-09 02:27:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/585183004/60001

[commit-bot: I haz the power, 2014-10-09 04:29:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 04:29:05]

Exceeded time limit waiting for builds to trigger.

[pdr., 2014-10-09 05:05:23]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-09 05:06:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/585183004/60001

[commit-bot: I haz the power, 2014-10-09 06:40:48]

Committed patchset #4 (id:60001) as 183441