| 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: | P2 | 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
Sjoerd Mulder
2005-10-24 07:43:10 PDT
Changed severity to critical because this bug prevents development of any kind of Ajax applications 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. Created attachment 4588 [details]
local copy of the test case
I believe there are workarounds for this -- we should find them as well as fixing the bug. 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. 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.
Given the workaround, downgrading "severity". This bug is also in Radar: <rdar://4327485> Created attachment 7078 [details]
Layout test
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()?
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. 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. 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>.
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. 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. Comment on attachment 7078 [details]
Layout test
Test looks good, but removing review flag for now since we don't have a fix yet.
*** Bug 18042 has been marked as a duplicate of this bug. *** *** Bug 5478 has been marked as a duplicate of this bug. *** I'm working on this. Created attachment 22268 [details]
Patch including test cases and ChangeLog
Created attachment 22269 [details]
Change the iteration in addStyleSheetCandidateNode to go from last to first
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) {
Fixed in r35172. |