Bug 60155

Summary: REGRESSION (r65868): createContextualFragment does not work with <style>
Product: WebKit Reporter: Xavier Claessens <xclaesse>
Component: DOMAssignee: 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:
Description Flags
test case
none
Patch
none
Adium HTML/JS template
none
Various Adium themes none

Xavier Claessens
Reported 2011-05-04 03:59:38 PDT
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.
Attachments
test case (530 bytes, text/html)
2011-05-04 10:59 PDT, Alexey Proskuryakov 		no flags
Details
Patch (6.01 KB, patch)
2011-05-12 00:39 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Adium HTML/JS template (10.42 KB, text/plain)
2011-05-12 10:27 PDT, Xavier Claessens 		no flags
Details
Various Adium themes (1.05 MB, application/x-gzip)
2011-05-13 13:43 PDT, Xavier Claessens 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2011-05-04 10:58:15 PDT
It seems working in Safari 5, but broken in current builds.
Alexey Proskuryakov
Comment 2 2011-05-04 10:59:04 PDT
Created attachment 92282 [details] test case
Alexey Proskuryakov
Comment 3 2011-05-04 11:43:55 PDT
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?
Eric Seidel (no email)
Comment 4 2011-05-04 11:48:50 PDT
I've testing minefield and they show an alert containing the expected html.
Eric Seidel (no email)
Comment 5 2011-05-04 11:50:09 PDT
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.
Adam Barth
Comment 6 2011-05-11 22:26:59 PDT
createContextualFragment has no spec and is sadness. Investigating.
Adam Barth
Comment 7 2011-05-11 22:32:46 PDT
<head> <body> <script> var range = document.createRange(); range.selectNode(document.body); var documentFragment = range.createContextualFragment("<style></style>XXX"); document.body.appendChild(documentFragment); </script>
Adam Barth
Comment 8 2011-05-11 23:56:29 PDT
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
Adam Barth
Comment 9 2011-05-12 00:39:25 PDT
Created attachment 93261 [details] Patch
Alexey Proskuryakov
Comment 10 2011-05-12 08:56:26 PDT
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.
Adam Barth
Comment 11 2011-05-12 10:02:44 PDT
> 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?
Xavier Claessens
Comment 12 2011-05-12 10:27:17 PDT
Created attachment 93300 [details] Adium HTML/JS template Here is the html file we use for adium themes, with JS code.
Adam Barth
Comment 13 2011-05-12 10:57:32 PDT
> Here is the html file we use for adium themes, with JS code. Awesome, thanks. I'll try converting that to a LayoutTest.
WebKit Commit Bot
Comment 14 2011-05-12 11:29:12 PDT
Comment on attachment 93261 [details] Patch Clearing flags on attachment: 93261 Committed r86364: <http://trac.webkit.org/changeset/86364>
WebKit Commit Bot
Comment 15 2011-05-12 11:29:18 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 16 2011-05-12 12:24:03 PDT
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.
Ademar Reis
Comment 17 2011-05-13 09:29:28 PDT
Revision r86364 cherry-picked into qtwebkit-2.2 with commit 92d1b15 <http://gitorious.org/webkit/qtwebkit/commit/92d1b15>
Xavier Claessens
Comment 18 2011-05-13 10:19:05 PDT
(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!
Adam Barth
Comment 19 2011-05-13 13:37:44 PDT
(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.
Xavier Claessens
Comment 20 2011-05-13 13:43:55 PDT
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.
Gustavo Noronha (kov)
Comment 21 2011-05-23 11:17:00 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.