Bug 12214 - REGRESSION: css image with relative path is not loaded, when both base element and dir attribute in the html tag are present
Summary: REGRESSION: css image with relative path is not loaded, when both base elemen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: PC OS X 10.4
: P1 Normal
Assignee: Nobody
URL: https://accountservices.passport.net/...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-01-11 11:11 PST by zalan
Modified: 2007-02-19 17:33 PST (History)
3 users (show)

See Also:


Attachments
simple test case (179 bytes, text/html)
2007-01-11 11:13 PST, zalan
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, zalan
darin: review-
Details | Formatted Diff | Diff
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, zalan
mjs: review-
Details | Formatted Diff | Diff
fix + test cases (28.04 KB, patch)
2007-02-14 18:49 PST, zalan
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 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.
Comment 1 zalan 2007-01-11 11:13:45 PST
Created attachment 12365 [details]
simple test case
Comment 2 zalan 2007-01-11 11:17:27 PST
Created attachment 12366 [details]
This fix updates m_elemSheet's href, whenever document's baseUrl changes.
Comment 3 Alexey Proskuryakov 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).
Comment 4 Alexey Proskuryakov 2007-01-11 12:48:05 PST
BTW, the original test case works in shipping Safari, so it's a regression/P1.
Comment 5 zalan 2007-01-11 12:52:30 PST
dynamic test case doesn't work in IE6 either.
Comment 6 zalan 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.)

Comment 7 Alexey Proskuryakov 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 :)
Comment 8 Darin Adler 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
Comment 9 zalan 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.
Comment 10 Darin Adler 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.
Comment 11 Mark Rowe (bdash) 2007-01-28 19:09:47 PST
<rdar://problem/4960263>
Comment 12 zalan 2007-02-13 15:15:25 PST
Created attachment 13156 [details]
baseURL fix and testcase added
Comment 13 Darin Adler 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!
Comment 14 zalan 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...
Comment 15 Maciej Stachowiak 2007-02-14 09:17:04 PST
Comment on attachment 13156 [details]
baseURL fix and testcase added

Changed to r- per Zalan's request.
Comment 16 Alexey Proskuryakov 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.
Comment 17 zalan 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)
Comment 18 zalan 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().
Comment 19 Darin Adler 2007-02-19 14:58:57 PST
Comment on attachment 13178 [details]
fix + test cases

r=me
Comment 20 Sam Weinig 2007-02-19 17:33:20 PST
Landed in r19716.