When html tag has dir attribute and the base element is present in the head, then images (like background image), in inline css, with relative path are not loaded. CSSStyleSheet is constructed (CSSStyleSheet* Document::elementSheet()) when <html dir="ltr"> tag is parsed. The issue here is that m_elemSheet's href does not get updated, when the document's base url is changed by the <base element. see the test case.
Created attachment 12365 [details] simple test case
Created attachment 12366 [details] This fix updates m_elemSheet's href, whenever document's baseUrl changes.
Created attachment 12368 [details] dynamic test Could this be too dynamic? The attached test case doesn't work in Firefox (haven't tested IE).
BTW, the original test case works in shipping Safari, so it's a regression/P1.
dynamic test case doesn't work in IE6 either.
ap, I can't find document.base in IDL Definition...all i can see is this: readonly attribute DOMString baseURI; I debugged webkit with the dynamic test case and the setBaseURL doesn't get called on document.base. (That's why the image does not show up even with my patch.)
Created attachment 12377 [details] dynamic test Fixed the test to actually set the base dynamically. The background image does appear in IE7 now! Haven't yet tested if it reloads previously loaded subresources when the base changes :)
Comment on attachment 12366 [details] This fix updates m_elemSheet's href, whenever document's baseUrl changes. Great fix! + if (m_elemSheet) + m_elemSheet.get()->setHref(m_baseURL); No need for the ".get()" herre. It can just be m_elemSheet->setHref(). It looks to me like this will not work if the style sheet has an @import rule in it. The @import rule will load based on the original style sheet href, not the updated one. This needs at least one layout test; maybe two because of the @import rule issue. I think it's OK to fix this even without a fix for the @import rule issue. review-, because of the lack of a test
Thanks for the review. i'll fix the patch and add a test case. >It looks to me like this will not work if the style sheet has an @import rule >in it. The @import rule will load based on the original style sheet href, not >the updated one. in case of external style sheet, I think the url in the @import rule is relative to the style sheet's url (unless it is an absolute url) so the base element in the .html should/can not alter it, while in case of inline style sheet, if i had something like this <body style="@import url(background_image_is_set_here.css);"> then i believe, my patch would update the inline stylesheet's href and load the .css from the right location. now, I'm wondering if you meant something completly different.
(In reply to comment #9) > in case of external style sheet, I think the url in the @import rule is > relative to the style sheet's url (unless it is an absolute url) so the base > element in the .html should/can not alter it, while in case of inline style > sheet, if i had something like this > <body style="@import url(background_image_is_set_here.css);"> then i believe, > my patch would update the inline stylesheet's href and load the .css from the > right location. > now, I'm wondering if you meant something completly different. I guess that's right; I'm not sure. I saw that CSSImportRule::insertedIntoParent() calls href() and I was wondering why it was OK to do so if the href() might change later when we parsed a base element.
<rdar://problem/4960263>
Created attachment 13156 [details] baseURL fix and testcase added
Comment on attachment 13156 [details] baseURL fix and testcase added Given our discussion of @import, I'm surprised you didn't also add @import tests. + m_elemSheet.get()->setHref(m_baseURL); I mentioned in the last review that you didn't need a get() here. Was there a reason you didn't remove it? Change looks fine, r=me, but I get a strange sense that you didn't pay attention to my comments last time!
I'm sorry, i was so focusing on the layouttest. I had difficulties to avoid http:// scheme. Could you r- it? I'll fix the patch...
Comment on attachment 13156 [details] baseURL fix and testcase added Changed to r- per Zalan's request.
(In reply to comment #14) > I had difficulties to avoid http:// scheme. Out of curiosity, why did you need to? We have a whole bunch of automated http tests - see, for example, http/tests/uri/css-href.php. Since they are served by Apache, they can use PHP, CGI, and a lot of other server technologies.
>Out of curiosity, why did you need to? We have a whole bunch of automated >http tests - see, for example, http/tests/uri/css-href.php. Since they >are served by Apache, they can use PHP, CGI, and a >lot of other server technologie You don't need to. I just prefer file:// when it comes to layouttest. (and iirc, it is also mentioned in the "Writing good test cases" section of the wiki)
Created attachment 13178 [details] fix + test cases I looked into the @import issue and, - @import inside inline <style (<body style="@import url(background_image_is_set_here.css);">) is not implemented by anyone. http://www.w3.org/TR/css-style-attr never made it. it is still working draft. - normal @import use case, like <head><style> @import... works without my patch. I still added this test case with the combination of <base element. I guess an extra test does not hurt. -removed the get().
Comment on attachment 13178 [details] fix + test cases r=me
Landed in r19716.