1357132 - Do we still need the style attr special-casing in nsGenericHTMLElement::CopyInnerTo?

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This was added in bug 137428 but I'm not sure why it's needed, really.  If it _is_ needed, I'm not sure why we don't need it for SVG/XUL style attributes.

Needs investigation.

It's _possible_ that this is a performance win (due to not having to reparse the style attr), of course.  Need to check.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The answer is that this is not needed.  In fact, it causes an observable bug: when doing importNode between documents with different base URIs, we should be reparsing the style attr (because the base URI changed) and are not.

I filed bug 1357645 on maybe improving the performance here, but I'm not seeing much of a performance impact from removing this thing in the form it's in right now.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the broken style attr special casing in HTML element cloning

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8859449 [details] [diff] [review]
Remove the broken style attr special casing in HTML element cloning

Review of attachment 8859449 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This causes a test failure in the content-security-policy/blink-contrib/inline-style-allowed-while-cloning-objects.sub.html WPT test.  As far as I can tell, this test does not match the spec at all, though it does match the implementations in Firefox/Safari/Chrome.  I filed https://github.com/w3c/web-platform-tests/issues/5614 on that and will wait for a response there before landing this, on the off chance that I misunderstood the spec or that the spec is wrong right now.  Would rather not change behavior twice.

Comment 5

[Andrew Overholt [:overholt]]

Not much happening at the WPT issue referenced in comment 4 or the related spec issue (https://github.com/w3c/webappsec-csp/issues/212) ...

I was going to mark this qf:p1 because it blocks bug 1357645 which is qf:p1 but based on comment 1, I'm thinking we maybe don't need this to be qf:anything so I'll leave it for now.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bzbarsky, could you have a look please?

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is blocked on some of the Google CSP folks actually responding in the spec issues linked above.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Christoph, would you be able to poke the CSP spec here to get it sorted out? See https://github.com/w3c/webappsec-csp/issues/212

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky] from comment #8)

Christoph, would you be able to poke the CSP spec here to get it sorted out? See https://github.com/w3c/webappsec-csp/issues/212

Tom, is this something you could take on?

Comment 10

[Tom Schuster (MoCo)]

I have been looking at this on and off, but understanding some of the details here is quite difficult. I think most of the complexity here doesn't actually reside on the CSP side. It's more of a DOM or style question.

Or do you just want me to ping the thread?

Comment 11

[Boris Zbarsky [:bzbarsky]]

There is no complexity except for the fact that going with a sane behavior makes a CSP test fail. See comment 4 and following....