It used to be way different, but probably most of the work could be delegated to Element::Clone or nsStyledElement.

From https://phabricator.services.mozilla.com/D44069?id=155649#inline-268227:

Yeah, please file a followup to remove this special-casing. I expect cloning used to look very different back when some of the attrs lived on the proto-element, but nowadays we should just delegate more of this to our superclass. Maybe we can put the HTML code on nsStyledElement, for example, since most of the complication there is for style attributes.

I'm a bit confused with the declaration bits in https://searchfox.org/mozilla-central/rev/7088fc958db5935eba24b413b1f16d6ab7bd13ea/dom/html/nsGenericHTMLElement.cpp#131.

As far as I can tell, the comment about "that will fail to reparse the string" is just not true. We do want to keep the style attribute because CSP would prevent that if we reparsed it and it would be blocked by CSP, right? The comment about cloning also doesn't seem true, we just keep a ref to the declaration.

XUL stuff is cloning the declaration. What kind of behavior do we want? I assume taking a ref and marking the declaration immutable ought to be fine for XUL as well... right?

Also what's the deal with other nsStyledElements that aren't nsGenericHTMLElements? Like when I clone an SVG element with style attributes, we block that due to CSP? That sounds inconsistent.

I suspect we could make Element::CopyInnerTo do what nsGenericHTMLElement::CopyInnerTo is doing with respect to style attributes. It'd be nice to not reparse all attribute for non-cross-document clones, though that may need a bit of auditing... Worth trying?

The attribute is definitely set in AfterSetAttr...

This contains an (intentional) behavior change, which is that we always copy
(i.e. don't reparse) style attributes, even across documents.

XUL and HTML already had this behavior. This makes stuff like SVG and MathML
consistent with that.

Sent a patch instead. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2031208166e8c05e1035bba4e8eda7a3f9fbc3f2 is a green try run... Though maybe a test for the CSP stuff would be on point. If you tell me where to cargo-cult from... maybe.

As far as I can tell, the comment about "that will fail to reparse the string" is just not true.

It was true when written; see https://searchfox.org/mozilla-central/rev/1152b3d656571e5388bb001fe772a4f6e5fbdd17/content/html/content/src/nsGenericHTMLElement.cpp and the ReparseStyleAttribute bits in SetDocument there (that was the equivalent of BindToTree back then). But yes, I agree that it no longer seems to be true, so we should remove it.

The comment about cloning also doesn't seem true

Indeed. That changed in bug 1373744 but the comment didn't get updated. :(

XUL stuff is cloning the declaration.

Probably predates the ability to refcount declarations.

I assume taking a ref and marking the declaration immutable ought to be fine for XUL as well... right?

I think so, yes.

Also what's the deal with other nsStyledElements that aren't nsGenericHTMLElements?

They get no love. :(

Though maybe a test for the CSP stuff would be on point. If you tell me where to cargo-cult from... maybe.

Maybe https://searchfox.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html ?

Looking at the patches now.

Thank you for cleaning this up!

https://hg.mozilla.org/mozilla-central/rev/bb7e6e6521e7
https://hg.mozilla.org/mozilla-central/rev/96884f510f32