1577721 - Remove special-casing in nsXULElement::Clone.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1577721 - Use AddListenerFor in nsXULElement::AfterSetAttr.

The attribute is definitely set in AfterSetAttr...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1577721 - Unify attribute cloning between XUL / HTML and everything else.

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Thank you for cleaning this up!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb7e6e6521e7
Use AddListenerFor in nsXULElement::AfterSetAttr. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/96884f510f32
Unify attribute cloning between XUL / HTML and everything else. r=bzbarsky

Comment 8

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18789 for changes under testing/web-platform/tests

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 10

[Dorel Luca [:dluca]]

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

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot