RESOLVED FIXED 12214
REGRESSION: css image with relative path is not loaded, when both base element and dir attribute in the html tag are present 
https://bugs.webkit.org/show_bug.cgi?id=12214
Summary REGRESSION: css image with relative path is not loaded, when both base elemen...
alan baradlay
Reported 2007-01-11 11:11:38 PST
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.
Attachments
simple test case (179 bytes, text/html)
2007-01-11 11:13 PST, alan baradlay 		no flags
Details
This fix updates m_elemSheet's href, whenever document's baseUrl changes. (2.53 KB, patch)
2007-01-11 11:17 PST, alan baradlay 		darin: review-
DetailsFormatted DiffDiff
dynamic test (227 bytes, text/html)
2007-01-11 12:46 PST, Alexey Proskuryakov 		no flags
Details
dynamic test (886 bytes, text/html)
2007-01-11 22:42 PST, Alexey Proskuryakov 		no flags
Details
baseURL fix and testcase added (15.46 KB, patch)
2007-02-13 15:15 PST, alan baradlay 		mjs: review-
DetailsFormatted DiffDiff
fix + test cases (28.04 KB, patch)
2007-02-14 18:49 PST, alan baradlay 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
alan baradlay
Comment 1 2007-01-11 11:13:45 PST
Created attachment 12365 [details] simple test case
alan baradlay
Comment 2 2007-01-11 11:17:27 PST
Created attachment 12366 [details] This fix updates m_elemSheet's href, whenever document's baseUrl changes.
Alexey Proskuryakov
Comment 3 2007-01-11 12:46:50 PST
Created attachment 12368 [details] dynamic test Could this be too dynamic? The attached test case doesn't work in Firefox (haven't tested IE).
Alexey Proskuryakov
Comment 4 2007-01-11 12:48:05 PST
BTW, the original test case works in shipping Safari, so it's a regression/P1.
alan baradlay
Comment 5 2007-01-11 12:52:30 PST
dynamic test case doesn't work in IE6 either.
alan baradlay
Comment 6 2007-01-11 14:03:05 PST
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.)
Alexey Proskuryakov
Comment 7 2007-01-11 22:42:30 PST
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 :)
Darin Adler
Comment 8 2007-01-13 07:33:53 PST
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
alan baradlay
Comment 9 2007-01-13 22:01:13 PST
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.
Darin Adler
Comment 10 2007-01-16 00:09:10 PST
(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.
Mark Rowe (bdash)
Comment 11 2007-01-28 19:09:47 PST
<rdar://problem/4960263>
alan baradlay
Comment 12 2007-02-13 15:15:25 PST
Created attachment 13156 [details] baseURL fix and testcase added
Darin Adler
Comment 13 2007-02-14 08:59:59 PST
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!
alan baradlay
Comment 14 2007-02-14 09:06:25 PST
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...
Maciej Stachowiak
Comment 15 2007-02-14 09:17:04 PST
Comment on attachment 13156 [details] baseURL fix and testcase added Changed to r- per Zalan's request.
Alexey Proskuryakov
Comment 16 2007-02-14 10:08:12 PST
(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.
alan baradlay
Comment 17 2007-02-14 10:19:44 PST
>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)
alan baradlay
Comment 18 2007-02-14 18:49:28 PST
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().
Darin Adler
Comment 19 2007-02-19 14:58:57 PST
Comment on attachment 13178 [details] fix + test cases r=me
Sam Weinig
Comment 20 2007-02-19 17:33:20 PST
Landed in r19716.
Note You need to log in before you can comment on or make changes to this bug.