Bug 60155 - REGRESSION (r65868): createContextualFragment does not work with <style>
Summary: REGRESSION (r65868): createContextualFragment does not work with <style>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Adam Barth
URL:
Keywords: Regression
Depends on:
Blocks: 41115
  Show dependency treegraph
 
Reported: 2011-05-04 03:59 PDT by Xavier Claessens
Modified: 2011-05-23 11:17 PDT (History)
9 users (show)

See Also:


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 Details | Formatted Diff | Diff
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

Note You need to log in before you can comment on or make changes to this bug.
Description Xavier Claessens 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.
Comment 1 Alexey Proskuryakov 2011-05-04 10:58:15 PDT
It seems working in Safari 5, but broken in current builds.
Comment 2 Alexey Proskuryakov 2011-05-04 10:59:04 PDT
Created attachment 92282 [details]
test case
Comment 3 Alexey Proskuryakov 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?
Comment 4 Eric Seidel 2011-05-04 11:48:50 PDT
I've testing minefield and they show an alert containing the expected html.
Comment 5 Eric Seidel 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.
Comment 6 Adam Barth 2011-05-11 22:26:59 PDT
createContextualFragment has no spec and is sadness.  Investigating.
Comment 7 Adam Barth 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>
Comment 8 Adam Barth 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
Comment 9 Adam Barth 2011-05-12 00:39:25 PDT
Created attachment 93261 [details]
Patch
Comment 10 Alexey Proskuryakov 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.
Comment 11 Adam Barth 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?
Comment 12 Xavier Claessens 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.
Comment 13 Adam Barth 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.
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2011-05-12 11:29:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Commit Bot 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.
Comment 17 Ademar Reis 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>
Comment 18 Xavier Claessens 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!
Comment 19 Adam Barth 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.
Comment 20 Xavier Claessens 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.
Comment 21 Gustavo Noronha (kov) 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