Bug 12214

Summary:

REGRESSION: css image with relative path is not loaded, when both base element and dir attribute in the html tag are present

Product:

WebKit

Reporter:

alan <zalan>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alice.barraclough, ap, KwhiteRight

Priority:

Keywords:

InRadar, Regression

Version:

420+

Hardware:

OS:

OS X 10.4

URL:

https://accountservices.passport.net/pp450/cache/Cobranding.srf

Attachments:

Description	Flags
simple test case	none
This fix updates m_elemSheet's href, whenever document's baseUrl changes.	darin: review-
dynamic test	none
dynamic test	none
baseURL fix and testcase added	mjs: review-
fix + test cases	darin: review+

alan

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	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	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, alan	mjs: review-	Details Formatted Diff Diff
fix + test cases (28.04 KB, patch) 2007-02-14 18:49 PST, alan	darin: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2007-01-11 11:13:45 PST

Created attachment 12365 [details] simple test case

alan

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

Comment 5 2007-01-11 12:52:30 PST

dynamic test case doesn't work in IE6 either.

alan

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

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

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

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

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

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.