| Summary: | REGRESSION (r65868): createContextualFragment does not work with <style> | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Xavier Claessens <xclaesse> | ||||||||||
| Component: | DOM | Assignee: | Adam Barth <abarth> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, ademar, aestes, ap, commit-queue, eric, gustavo, ian, rniwa | ||||||||||
| Priority: | P1 | Keywords: | Regression | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 41115 | ||||||||||||
| Attachments: |
|
||||||||||||
It seems working in Safari 5, but broken in current builds. Created attachment 92282 [details]
test case
Regressed with the switch to HTML5 TreeBuilder for fragment parsing. Works as expected in Firefox 4, and generally seems like a very reasonable thing to do. Adam, Eric, would you be willing to look into this one? I've testing minefield and they show an alert containing the expected html. I suspect we're moving the style node to the head of the fragment documetn during parsing and then only returning the body contents (per the spec). I'd have to confirm. It's possible we're supposd to disable that move-to-head behavior during fragment parse. createContextualFragment has no spec and is sadness. Investigating. <head>
<body>
<script>
var range = document.createRange();
range.selectNode(document.body);
var documentFragment = range.createContextualFragment("<style></style>XXX");
document.body.appendChild(documentFragment);
</script>
The style element gets eaten by Element::deprecatedCreateContextualFragment. It ends up in a <head> element of the fragment but gets deleted on https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L173 Created attachment 93261 [details]
Patch
Comment on attachment 93261 [details]
Patch
It would be great to actually have a PASS/FAIL test for this. It's very hard to ensure that we don't break Adium themes if all we have is a dump of largely unrelated markup, with a related part buried somewhere in the middle.
> It would be great to actually have a PASS/FAIL test for this. It's very hard to ensure that we don't break Adium themes if all we have is a dump of largely unrelated markup, with a related part buried somewhere in the middle. What would be ideal, from the point of view of not breaking Adium themes, is to have a test that's a more faithful representation of what they're trying to do. The tests in this patch are just the reductions I used to reverse engineer Firefox's behavior. @Xavier: Is the code you posted in Comment #0 what you'd like us to test, or is there something more we should be sure not to break? Created attachment 93300 [details]
Adium HTML/JS template
Here is the html file we use for adium themes, with JS code.
> Here is the html file we use for adium themes, with JS code.
Awesome, thanks. I'll try converting that to a LayoutTest.
Comment on attachment 93261 [details] Patch Clearing flags on attachment: 93261 Committed r86364: <http://trac.webkit.org/changeset/86364> All reviewed patches have been landed. Closing bug. The commit-queue encountered the following flaky tests while processing attachment 93261 [details]: http/tests/xmlhttprequest/failed-auth.html bug 51835 (author: ap@webkit.org) The commit-queue is continuing to process your patch. Revision r86364 cherry-picked into qtwebkit-2.2 with commit 92d1b15 <http://gitorious.org/webkit/qtwebkit/commit/92d1b15> (In reply to comment #17) > Revision r86364 cherry-picked into qtwebkit-2.2 with commit 92d1b15 <http://gitorious.org/webkit/qtwebkit/commit/92d1b15> This seems to be merged into qtwebkit, however I'm using webkit gtk, is it already merged into it? Where is its git repository? Thanks for the quick fix! (In reply to comment #13) > > Here is the html file we use for adium themes, with JS code. > > Awesome, thanks. I'll try converting that to a LayoutTest. Hum... There's lots of cool stuff to test in there, but it's not clear to me how to turn it into a test without more of the system. Created attachment 93502 [details]
Various Adium themes
In case it can help, I've attached a tarball containing various adium themes. Those are the one shipped build-in with adium itself on macos. They are working fine in empathy as well using webkit-gtk. The only issue I've seen is this bug.
Feel free to make unit tests out of this, but I'm not sure of the copyright/licence though. The tarball is files stolen directly from adium's tarball.
(In reply to comment #18) > This seems to be merged into qtwebkit, however I'm using webkit gtk, is it already merged into it? Where is its git repository? We'll merge it. We've been using a branch in svn for the 1.4.x series: http://trac.webkit.org/browser/releases/WebKitGTK/webkit-1.4 |
I'm trying to add support for Adium chat themes in Empathy using Webkit GTK. One of the JS function in the theme's HTML fails when running in webkit GTK but I guess it's working fine with webkit on MacOS: function setStylesheet( id, url ) { var code = "<style id=\"" + id + "\" type=\"text/css\" media=\"screen,print\">"; if( url.length ) code += "@import url( \"" + url + "\" );"; code += "</style>"; var range = document.createRange(); var head = document.getElementsByTagName( "head" ).item(0); range.selectNode( head ); var documentFragment = range.createContextualFragment( code ); head.removeChild( document.getElementById( id ) ); head.appendChild( documentFragment ); } When debugging, I've found that documentFragment's childNodes is empty. However if I do s/style/div/ in the above function, documentFragment's childNodes correctly contains a one HTMLDivElement. So I came to the conclusion that createContextualFragment("<style></style>") is broken in webkit-gtk.