CSS: SVG use elements replicate updates to style

CSS: SVG use elements replicate updates to style

When an SVG element's style is being updated, shadow instances created by <use> elements must also be updated.

We weren't previously propagating the change until the the next time the <use> element's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class change, a class change or a CSS animation.

BUG=166438, 355359, 484552, 637310
Committed: https://crrev.com/98eafa054ec08b60ab06632b39c543982c20dfdc
Cr-Commit-Position: refs/heads/master@{#413048}

[Eric Willigers, 2016-08-18 06:45:03]

Patchset #1 (id:1) has been deleted

[Eric Willigers, 2016-08-18 06:50:56]

Description was changed from

==========
[WIP] CSS: Propagate SVG style updates to use elements


BUG=484552
==========

to

==========
CSS: Propagate SVG style updates to use elements

When an SVG element's style is being updated, <use> clones of that element must
also be updated.

We weren't previously propagating the change until the the next time the <use>
clone's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310
==========

[Eric Willigers, 2016-08-18 06:50:56]

ericwilligers@chromium.org changed reviewers:
+ dstockwell@chromium.org, fs@opera.com

[Eric Willigers, 2016-08-18 06:51:03]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 06:51:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 08:09:00]

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

[commit-bot: I haz the power, 2016-08-18 08:09:01]

Dry run: This issue passed the CQ dry run.

[fs, 2016-08-18 09:09:11]

fs@opera.com changed reviewers:
+ rune@opera.com

[fs, 2016-08-18 09:09:11]

+rune

[rune, 2016-08-18 21:09:07]

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:718:
element->setNeedsStyleRecalc(SubtreeStyleChange, reason);
So there's an 1:n relationship between each svg element in the original subtree
and each and every svg element copy in the use shadow trees?

Could we make this cheaper than SubtreeStyleChange on the <use> element? Would
it be incorrect to just set the StyleChangeType on the specific instances
instead of nuking style for the the whole subtree for every change?

It's not possible to modify the use shadow tree to be different from the
referenced subtree, right?

IIUC, the computed styles for the copied subtree might be different because the
"used" elements will inherit from the <use> element. Is that right?

[rune, 2016-08-18 21:15:29]

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:712: return;
I think I'd move the code below into a separate method on SVGElement.

[fs, 2016-08-18 21:21:17]

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:718:
element->setNeedsStyleRecalc(SubtreeStyleChange, reason);
On 2016/08/18 at 21:09:07, rune wrote:
> So there's an 1:n relationship between each svg element in the original
subtree and each and every svg element copy in the use shadow trees?

Yes.

> Could we make this cheaper than SubtreeStyleChange on the <use> element? Would
it be incorrect to just set the StyleChangeType on the specific instances
instead of nuking style for the the whole subtree for every change?

I was hoping we could do something like that. Haven't been able to come up with
a counter-example to that, but maybe Eric has?

> It's not possible to modify the use shadow tree to be different from the
referenced subtree, right?

Correct.

> IIUC, the computed styles for the copied subtree might be different because
the "used" elements will inherit from the <use> element. Is that right?

Yes.

[Eric Willigers, 2016-08-18 22:22:59]

Description was changed from

==========
CSS: Propagate SVG style updates to use elements

When an SVG element's style is being updated, <use> clones of that element must
also be updated.

We weren't previously propagating the change until the the next time the <use>
clone's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310
==========

to

==========
CSS: SVG use elements replicate updates to style

When an SVG element's style is being updated, shadow instances created by <use>
elements must also be updated.

We weren't previously propagating the change until the the next time the <use>
element's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310
==========

[Eric Willigers, 2016-08-18 22:27:00]

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:712: return;
On 2016/08/18 21:15:28, rune wrote:
> I think I'd move the code below into a separate method on SVGElement.

Done.

https://codereview.chromium.org/2251073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:718:
element->setNeedsStyleRecalc(SubtreeStyleChange, reason);
> just set the StyleChangeType on the specific instances instead
> of nuking style for the the whole subtree for every change?

Done. All my tests still pass. I should have tried it earlier.

[rune, 2016-08-18 23:43:07]

lgtm with nits.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo-expected.html
(right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo-expected.html:4:
<meta charset="utf-8">
Drop html/head/body/meta.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo.html
(right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo.html:4:
<meta charset="utf-8">
drop html/head/body. Do you need the meta charset?

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.cpp:992: }
Unnecessary { }

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.h:183: void
instancesNeedsStyleRecalc(StyleChangeType, const StyleChangeReasonForTracing&);
Name sounds like returning a bool to me. How about
setNeedsStyleRecalcForInstances?

[rune, 2016-08-18 23:43:07]

lgtm with nits.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo-expected.html
(right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo-expected.html:4:
<meta charset="utf-8">
Drop html/head/body/meta.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo.html
(right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo.html:4:
<meta charset="utf-8">
drop html/head/body. Do you need the meta charset?

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.cpp:992: }
Unnecessary { }

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.h:183: void
instancesNeedsStyleRecalc(StyleChangeType, const StyleChangeReasonForTracing&);
Name sounds like returning a bool to me. How about
setNeedsStyleRecalcForInstances?

[Eric Willigers, 2016-08-19 00:44:19]

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo-expected.html
(right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo-expected.html:4:
<meta charset="utf-8">
On 2016/08/18 23:43:06, rune wrote:
> Drop html/head/body/meta.

Done.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo.html
(right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/use-replicates-changes-pseudo.html:4:
<meta charset="utf-8">
On 2016/08/18 23:43:07, rune wrote:
> drop html/head/body. Do you need the meta charset?

Done.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.cpp:992: }
On 2016/08/18 23:43:07, rune wrote:
> Unnecessary { }

Done.

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/2251073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGElement.h:183: void
instancesNeedsStyleRecalc(StyleChangeType, const StyleChangeReasonForTracing&);
On 2016/08/18 23:43:07, rune wrote:
> Name sounds like returning a bool to me. How about
> setNeedsStyleRecalcForInstances?

Done.

[Eric Willigers, 2016-08-19 00:44:35]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2016-08-19 00:44:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/2251073002/#ps60001
(title: "tidy")

[commit-bot: I haz the power, 2016-08-19 00:45:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 04:26:10]

Description was changed from

==========
CSS: SVG use elements replicate updates to style

When an SVG element's style is being updated, shadow instances created by <use>
elements must also be updated.

We weren't previously propagating the change until the the next time the <use>
element's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310
==========

to

==========
CSS: SVG use elements replicate updates to style

When an SVG element's style is being updated, shadow instances created by <use>
elements must also be updated.

We weren't previously propagating the change until the the next time the <use>
element's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310
==========

[commit-bot: I haz the power, 2016-08-19 04:26:11]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-19 04:28:18]

Description was changed from

==========
CSS: SVG use elements replicate updates to style

When an SVG element's style is being updated, shadow instances created by <use>
elements must also be updated.

We weren't previously propagating the change until the the next time the <use>
element's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310
==========

to

==========
CSS: SVG use elements replicate updates to style

When an SVG element's style is being updated, shadow instances created by <use>
elements must also be updated.

We weren't previously propagating the change until the the next time the <use>
element's style happened to be recalculated.

For example, this bug has been reported with style updates due to a pseudo-class
change, a class change or a CSS animation.

BUG=166438,355359,484552,637310

Committed: https://crrev.com/98eafa054ec08b60ab06632b39c543982c20dfdc
Cr-Commit-Position: refs/heads/master@{#413048}
==========

[commit-bot: I haz the power, 2016-08-19 04:28:19]

Patchset 3 (id:??) landed as
https://crrev.com/98eafa054ec08b60ab06632b39c543982c20dfdc
Cr-Commit-Position: refs/heads/master@{#413048}