RESOLVED FIXED 5476
Dynamically adding <link>/<style> for CSS style sheet outside <head> fails to load style sheet 
https://bugs.webkit.org/show_bug.cgi?id=5476
Summary Dynamically adding <link>/<style> for CSS style sheet outside <head> fails to...
Sjoerd Mulder
Reported 2005-10-24 07:43:10 PDT
At Backbase we are trying to write support for Safari but we found the following bug: If you try to add a <link> tag dynamicly anywhere inside the DOM-tree throught createElement Safari does nothing (2.x and ToT)
Attachments
local copy of the test case (687 bytes, text/html)
2005-11-04 07:23 PST, Darin Adler 		no flags
Details
Workaround - append to head element (813 bytes, text/html)
2005-11-04 08:14 PST, Mark Malone 		no flags
Details
Layout test (3.29 KB, text/plain)
2006-03-15 01:34 PST, Tim Omernick 		no flags
Details
Proposed patch (1.29 KB, patch)
2006-03-15 01:45 PST, Tim Omernick 		hyatt: review-
DetailsFormatted DiffDiff
Patch including test cases and ChangeLog (21.12 KB, patch)
2008-07-14 13:14 PDT, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Change the iteration in addStyleSheetCandidateNode to go from last to first (21.17 KB, patch)
2008-07-14 13:39 PDT, Dave Hyatt 		sam: review+
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Sjoerd Mulder
Comment 1 2005-10-25 01:32:08 PDT
Changed severity to critical because this bug prevents development of any kind of Ajax applications
Darin Adler
Comment 2 2005-11-04 07:23:17 PST
I don't think that "because it affects my product" is really something that changes severity to "critical", but that doesn't matter much, because we don't use severity in a consistent way anyway. This is a high priority bug to fix.
Darin Adler
Comment 3 2005-11-04 07:23:45 PST
Created attachment 4588 [details] local copy of the test case
Darin Adler
Comment 4 2005-11-04 08:11:58 PST
I believe there are workarounds for this -- we should find them as well as fixing the bug.
Darin Adler
Comment 5 2005-11-04 08:13:52 PST
My tests on TOT show the <link> elements are being added to the DOM tree, although the stylesheets are not being loaded. So I think this is a similar issue to the recent one where we made added <script> tags trigger loading of the scripts.
Mark Malone
Comment 6 2005-11-04 08:14:53 PST
Created attachment 4592 [details] Workaround - append to head element LINK elements created with createElement and appended to the document head works as required. This appears to only be an issue when appending them to the body.
Darin Adler
Comment 7 2005-11-04 08:36:21 PST
Given the workaround, downgrading "severity".
Sjoerd Mulder
Comment 8 2006-02-03 00:36:08 PST
This bug is also in Radar: <rdar://4327485>
Tim Omernick
Comment 9 2006-03-15 01:34:47 PST
Created attachment 7078 [details] Layout test
Tim Omernick
Comment 10 2006-03-15 01:45:26 PST
Created attachment 7080 [details] Proposed patch I've attached a patch which passes the test case and layout test, and does not regress any of our layout tests. I'm not sure if this is the best way to fix this. Would it be a performance problem to deepen our search for <link> elements in DocumentImpl::recalcStyleSelector()?
Tim Omernick
Comment 11 2006-03-15 02:06:13 PST
The patch I attached also fixes 5478. If the patch (or a similar one which also fixes 5478) is accepted, then that bug should be marked as a duplicate of this one.
Maciej Stachowiak
Comment 12 2006-03-15 03:32:26 PST
Needs performance testing. We may have to replace the scan with maintaining a data structure of <link> and <style> elements in the document, since a linear scan of the document on style recalc could be expensive on heavily animated sites. At parse time you could just append, if you later dynamically insert or remove one you could either recompute the list in document order immediately, or mark it dirty and do a document scan later. Maybe the attached patch is ok if it doesn't affect performance in practice, but I think that needs testing. You would need a test case that recalculates style a lot, such as a DHTML animation done by modifying styles, such as by changing an absolute positioned element's top and left properties.
Dave Hyatt
Comment 13 2006-03-16 02:24:43 PST
Comment on attachment 7080 [details] Proposed patch It is not an acceptable solution to crawl the entire document looking for stylesheets. It is also completely invalid HTML to support <link> outside of the head by the way (this is in case the developer of the site in question is paying attention and cares enough to write HTML code that doesn't suck). I would suggest implementing the same fix that was done for <style> elements, which are also illegal outside of the head. Namely you should move the <link> element inside the head when the error condition is encountered while parsing. See htmlparser.cpp line 343 for how we do this for both <title> and <style>.
Dave Hyatt
Comment 14 2006-03-16 02:32:17 PST
Oops. Looks like we do deal with <link> when parsing. Disregard my previous comment. This is only an issue if you create the element via the DOM and insert it dynamically. I'd be inclined to make this a quirk if possible, since it's just totally broken to honor these elements outside the head, especially when created via DOM calls that could occur in strict XHTML. If it turns out other browsers, including WinIE, do support this capability, I would check to see if they are moving the elements into the <head>, and if so, do that.
Dave Hyatt
Comment 15 2006-03-16 02:41:36 PST
I tested in Firefox, and they leave the link element where it is. This is incredibly problematic, since you have to preserve the order of the stylesheets but they could be anywhere, and walking the whole document is not acceptable. Even maintaining a list would be difficult, since order is relevant, and someone could insert a link element in between two other link elements. I don't know how much work we want to do here to support such an obviously broken HTML construct when such a trivial workaround exists.
Darin Adler
Comment 16 2006-03-17 19:02:19 PST
Comment on attachment 7078 [details] Layout test Test looks good, but removing review flag for now since we don't have a fix yet.
Dave Hyatt
Comment 17 2008-04-25 08:59:17 PDT
*** Bug 18042 has been marked as a duplicate of this bug. ***
Dave Hyatt
Comment 18 2008-04-25 08:59:46 PDT
*** Bug 5478 has been marked as a duplicate of this bug. ***
Dave Hyatt
Comment 19 2008-07-11 13:06:13 PDT
I'm working on this.
Dave Hyatt
Comment 20 2008-07-14 13:14:46 PDT
Created attachment 22268 [details] Patch including test cases and ChangeLog
Dave Hyatt
Comment 21 2008-07-14 13:39:14 PDT
Created attachment 22269 [details] Change the iteration in addStyleSheetCandidateNode to go from last to first
Sam Weinig
Comment 22 2008-07-14 14:06:14 PDT
Comment on attachment 22269 [details] Change the iteration in addStyleSheetCandidateNode to go from last to first You can use an early return here to + if (createdByParser || m_styleSheetCandidateNodes.isEmpty()) + m_styleSheetCandidateNodes.add(node); + else { + // Determine an appropriate insertion point. Though this works, it might be cleaner to put the for-loop in an if-statement instead. + if (!matchAuthorAndUserStyles) + end = begin; + for (ListHashSet<Node*>::iterator it = begin; it != end; ++it) {
Dave Hyatt
Comment 23 2008-07-14 15:10:32 PDT
Fixed in r35172.
Note You need to log in before you can comment on or make changes to this bug.