Bug 5476

Summary:

Dynamically adding <link>/<style> for CSS style sheet outside <head> fails to load style sheet

Product:

WebKit

Reporter:

Sjoerd Mulder <sjoerdmulder>

Component:

DOM

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, ian, yuzhu.shen

Priority:

Keywords:

InRadar

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

URL:

http://projects.backbase.com/SafariBugs/link.html

Bug Depends on:

Bug Blocks:

6628

Attachments:

Description	Flags
local copy of the test case	none
Workaround - append to head element	none
Layout test	none
Proposed patch	hyatt: review-
Patch including test cases and ChangeLog	none
Change the iteration in addStyleSheetCandidateNode to go from last to first	sam: review+

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-	Details Formatted Diff Diff
Patch including test cases and ChangeLog (21.12 KB, patch) 2008-07-14 13:14 PDT, Dave Hyatt	no flags	Details Formatted Diff Diff
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+	Details Formatted Diff Diff
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.