Bug 17051 – safari should treat "url()" as a valid CSS value

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Anantha Keesara]

I .Steps:
-----------
1. Go to: http://jobs.virginmedia.com/UK/JobSeeker/Jobs/JobFindAdv.aspx


II. Issue:
-----------------
In the "Advanced Search" section, notice that half of the table is displaying with a colored background. 

III. Expected: There should not be a colored background.

IV. Other browsers:
In FF, IE, and Opera, there's no background color

V. Webkit nightly tested: r29807

Attached is a screenshot and reduction

[Anantha Keesara]

Created attachment 18745 [details]
reduction

[Anantha Keesara]

Created attachment 18746 [details]
screenshot

[Mark Rowe (bdash)]

What is the reduction supposed to show?  It looks identical in both WebKit and Firefox to me.

[Alexey Proskuryakov]

Since we don't have a "gradient_wide.jpg" file at bugs.webkit.org, it's not surprising that the reduction doesn't work. Having a complete test (archived into a single file or deployed on some server) in addition to this  demonstration would probably help screeners.

[Anantha Keesara]

Created attachment 18770 [details]
gradient_wide.jpg image

[Anantha Keesara]

oops.. My bad.. Sorry about that..
please see the testcase at: http://help.improve.safari.googlepages.com/test.html

[Eric Seidel (no email)]

So the real bug is that other browsers seem to treat url() as a valid enough background: value to not ignore the property complete, and thus clear the url for the background image on the second declaration.  Thus ending up with no background.  We ignore the "url()" and thus respect the first background: value.  I expect this is a rather minor compat issue.

[Eric Seidel (no email)]

Created attachment 18784 [details]
reduced test case

[Darin Fisher (:fishd, Google)]

Eric, I don't think your analysis is quite correct.  If you bring up the DOM inspector in Firefox, you'll see that it is using the URL of the document as the background image.  (Look at the computed style.)  So, it is treating url() as equivalent to the URL of the document.

[Eric Seidel (no email)]

(In reply to comment #9)
> Eric, I don't think your analysis is quite correct.  If you bring up the DOM
> inspector in Firefox, you'll see that it is using the URL of the document as
> the background image.  (Look at the computed style.)  So, it is treating url()
> as equivalent to the URL of the document.
> 

I agree, your analysis is more complete.  My statement is also true though.  We seem to ignore url() (probably at the parser level) and thus discard the "background:" property completely.  We do correctly resolve all url() values against the document URL in other case AFAIK.

[Eric Seidel (no email)]

This is likely the code causing the bug:

bool CSSParser::parseBackgroundImage(CSSValue*& value)
{
    if (valueList->current()->id == CSS_VAL_NONE) {
        value = new CSSImageValue();
        return true;
    }
    if (valueList->current()->unit == CSSPrimitiveValue::CSS_URI) {
        String uri = parseURL(domString(valueList->current()->string));
        if (!uri.isEmpty())
            value = new CSSImageValue(KURL(styleElement->baseURL().deprecatedString(), uri.deprecatedString()).string(),
                                         styleElement);
        return true;
    }
    return false;
}

valueList->current()->string will be the empty string in this case.

[Eric Seidel (no email)]

The fix is probably to change !uri.isEmpty() to !uri.isNull() since it looks like parseURL returns String() on error.

[Eric Seidel (no email)]

Created attachment 18974 [details]
another reduced test case

[Eric Seidel (no email)]

I would also recommend changing the name of parseURL when fixing this.  Change it to something like extractURLFromCSSURL or similar.

[Eric Seidel (no email)]

*** Bug 17288 has been marked as a duplicate of this bug. ***

[Bradley Meck]

Created attachment 19975 [details]
Proposed and tested on windows patch for empty url unit in CSS

Swapped the isEmpty for isNull as suggested, taking a look at parseUrl I do not feel it should change names as it is a universal way to access a Url that is done here.

[Julien Chaffraix]

>$ svn-create-patch
>?      WebKitLibraries/win/include/TargetConditionals.h
>?      WebKitLibraries/win/include/stdint.h
>?      WebKitLibraries/win/include/AssertMacros.h
>?      WebKitLibraries/win/include/stdbool.h
>?      WebKitLibraries/win/include/unistd.h
>?      WebKitLibraries/win/include/ConditionalMacros.h
>?      WebKitLibraries/win/include/AvailabilityMacros.h
>?      WebKitLibraries/win/include/xlocale.h
>?      WebKitLibraries/win/include/SafariTheme/SafariTheme.h
>?      WebKitLibraries/win/lib/CoreFoundation.lib
>?      WebKitLibraries/win/lib/CFNetwork.lib
>?      WebKitLibraries/win/lib/CoreGraphics.lib
>?      WebKit/win/WebKit.vcproj/tmp.obj
>?      WebKit/win/WebKit.vcproj/Thumbs.db
>?      BugsSite/Thumbs.db
>?      WebCore/Resources/Thumbs.db
>?      WebCore/css/CSSPropertyNames.cpp
>?      WebCore/css/CSSValueKeywords.gperf
>?      WebCore/css/CSSPropertyNames.gperf
>?      WebCore/css/CSSValueKeywords.c
>?      WebCore/WebCore.vcproj/tmp.obj
>?      LayoutTests/platform/win/css2.1/t17051-url-empty-expected.txt
>?      WebKitTools/WinLauncher/Thumbs.db
>?      WebKitTools/WinLauncher/WinLauncher.vcproj.MARKTHREEDESK.Bradley.user
>?      WebKitTools/CodeCoverage/Thumbs.db

We do not include the command line and the files not tracked by svn (it is only a reminder in case you have forgotten a file).

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog   (revision 31235)
>+++ WebCore/ChangeLog   (working copy)
>@@ -1,3 +1,17 @@
>[phone]  Bradley  <set EMAIL_ADDRESS environment variable>
>+

You should set the EMAIL_ADDRESS variable (maybe CHANGE_LOG_NAME too).

>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: css2.1/t17051-url-empty.html
>+
>+        * css/CSSHelper.cpp:
>+        (WebCore::parseURL):
>+        * css/CSSParser.cpp:
>+        (WebCore::CSSParser::parseValue):
>+        (WebCore::CSSParser::parseBackgroundImage):
>+        (WebCore::CSSParser::parseBorderImage):
>+        * css/makevalues.pl:
>+

See the other entries in the ChangeLog : it usually starts with the bug number and the bug summary (or at least the link to the bug).
Some explanations of the changes could also be helpful.

> 2008-03-22  Marco Barisione  <[e-mail]>
>
>         Reviewed by Darin Adler.
>Index: WebCore/css/CSSHelper.cpp
>===================================================================
>--- WebCore/css/CSSHelper.cpp   (revision 31183)
>+++ WebCore/css/CSSHelper.cpp   (working copy)
>@@ -43,6 +43,7 @@ String parseURL(const String& url)
>     while (l > 0 && (*i)[o + l - 1] <= ' ')
>         --l;
>
>+       //Check for "url(" at beginning and ")" at the end of url
>     if (l >= 5
>             && ((*i)[o] == 'u' || (*i)[o] == 'U')
>             && ((*i)[o + 1] == 'r' || (*i)[o + 1] == 'R')

The comment should have the same indentation as the following 'if'. We put a space between the "//" and the start of the comment too.

>Index: WebCore/css/CSSParser.cpp
>===================================================================
>--- WebCore/css/CSSParser.cpp   (revision 31183)
>+++ WebCore/css/CSSParser.cpp   (working copy)
>@@ -853,10 +853,10 @@ bool CSSParser::parseValue(int propId, b
>             } else if(strict && nrcoords == 2)
>                 hotspot = IntPoint(coords[0], coords[1]);
>             if (strict || coords.size() == 0) {
>-                if (!uri.isEmpty()) {
>+                if (!uri.isNull()) {
>                     list->append(new CSSCursorImageValue(KURL(styleElement->baseURL(), uri).string(),
>                         hotspot, styleElement));
>-                }
>+                               }

Indentation issue here too.

>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog       (revision 31235)
>+++ LayoutTests/ChangeLog       (working copy)
>@@ -1,3 +1,9 @@
>[phone]  Bradley  <set EMAIL_ADDRESS environment variable>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * css2.1/t17051-url-empty.html: Added.
>+

Same issues as the other ChangeLog.

> 2008-03-22  Eric Seidel  <[e-mail]>
>
>         Update a (passing) result I missed in my last checkin.  No review.
>Index: LayoutTests/css2.1/t17051-url-empty.html
>===================================================================
>--- LayoutTests/css2.1/t17051-url-empty.html    (revision 0)
>+++ LayoutTests/css2.1/t17051-url-empty.html    (revision 0)
>@@ -0,0 +1,13 @@
>+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
>+<html xmlns="http://www.w3.org/1999/xhtml" lang="utf-8"><head>
>+<style type="text/css">
>+body {
>+    background: red url('support/cat.png');
>+    background: green url();
>+}
>+</style>
>+</head>
>+<body>
>+There should be no cats in the background.
>+</body>
>+</html>
>
>Property changes on: LayoutTests/css2.1/t17051-url-empty.html
>___________________________________________________________________
>Name: svn:executable
>   + *

You are missing the results for the test case (see http://webkit.org/coding/contributing.html for how to generate them).

[Mark Rowe (bdash)]

Comment on attachment 19975 [details]
Proposed and tested on windows patch for empty url unit in CSS

Marking as r- per Julien's feedback on the patch.  Please address the issues that he pointed out and attach an updated version of the patch.

[Bradley Meck]

Created attachment 21501 [details]
Revised Patch

Changed the test for Bug #11221 because url() is valid and the test assumed otherwise.

[Bradley Meck]

Comment on attachment 21501 [details]
Revised Patch

Changelog:

2008-06-04  Bradley Meck  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        Test: css2.1/t17051-url-empty.html

        * ChangeLog:
        * css/CSSParser.cpp:
        (WebCore::CSSParser::parseValue):
        (WebCore::CSSParser::parseFillImage):
        (WebCore::CSSParser::parseBorderImage):

[Rob Buis]

Comment on attachment 21501 [details]
Revised Patch

R- because of ChangeLogs, Bradley is working on fixing them

[Bradley Meck]

Created attachment 21520 [details]
Hopefully properly formatted patch

Changelogs are good I hope...

[Darin Adler]

Comment on attachment 21520 [details]
Hopefully properly formatted patch

Change looks fine. Patch is still messed up though:

> 2008-06-05  Bradley Meck  <set EMAIL_ADDRESS environment variable>

This line should have an email address, not the error message in brackets. You can fix it yourself. The prepare-ChangeLog script is there to just help you write the log, there's nothing sacred about what it writes out and you should edit by hand to improve what it makes automatically.

>         Reviewed by NOBODY (OOPS!).
> 
>         Test: css2.1/t17051-url-empty.html
> 
>         * css/CSSParser.cpp:
>         (WebCore::CSSParser::parseValue):
>         (WebCore::CSSParser::parseFillImage):
>         (WebCore::CSSParser::parseBorderImage):

Here, you need comments explaining the change and usually mentioning the URL of the bug report as well. Please look at other ChangeLog entries to get an idea. Ideally you also have comments on each function explaining what changed there, although many people skip that step.

The part of your patch in the WebKit directory modifying WebKit.xcodeproj/project.pbxproj shouldn't be there at all. Please remove that. The best way is probably to use "svn revert" in that directory, or you can just edit them out of the patch before you post it.

The ChangeLog in the LayoutTests directory is messed up, with multiple entries. Please edit it and fix it. It should contain comments explaining what the changes are.

We normally don't add tests to the LayoutTests/css2.1 directory, since that's a set of tests made by others that we imported into the WebKit project. So ideally, the test would be in LayoutTests/fast/css instead. Also, it'd be best to give it an easier to understand name. I don't know why it's named t17051, although perhaps that's the name of the original test you derived it from.

> <div>This tests that the invalid cursor property value does not get applied. See Bug 11221. Updated to comply with Bug 17051.</div>

I don't think there's a need to add this "Updated" text to the invalid-cursor-property-crash.html test. It's fine to just correct the test without changing its descriptive text.

> if (style && style.cursor == "url("+document.location+"), auto")

We normally put spaces around operators like "+", and you should do so here.

review- mainly because the change logs are wrong and because the patch contains changes we don't want to land. If someone does want to land this and fix up all the minor problems that's OK. The actual substance of the change is fine.

[Bradley Meck]

Created attachment 21531 [details]
another attempt to format patch properly

Followed the email step by step I was sent... hopefully this is the last time :P

[Darin Adler]

Comment on attachment 21531 [details]
another attempt to format patch properly

WebCore/ChangeLog still doesn't include the URL of te bug.

WebKit/ChangeLog still has a gratuitous change in it (just a blank line, so harmless).

LayoutTests/ChangeLog mentions the ChangeLog itself as one of the files changed.

The LayoutTests patch includes the new test in its old path location, LayoutTests/css2.1/t17051-url-empty.html, and doesn't include the expected results for the new test.

I'm sorry -- I know you're trying hard to get this right but this is still not ready to land.

review- because of the LayoutTests problems. The other problems are minor.

[Bradley Meck]

Created attachment 21534 [details]
Yet another (hopeful this time)

Formatted stuff and changed location of new test

[Robert Blaut]

(In reply to comment #26)
> Created an attachment (id=21534) [edit]
> Yet another (hopeful this time)
> 
> Formatted stuff and changed location of new test
> 

Bradley, you haven't got generated laytout test result. After adding the new test to LaytoutTests/fast/css directory run the following command: "run-webkit-tests fast/css". It should generate expected test result for the new test: valid-url-empty.html. 

Also note that the test file doesn't have an empty line at end of file. Open the file valid-url-empty.html , place cursor after "</html>" string and press enter key. 

Also would be nice to add URL to this bug report in changelogs. For example: 

"3         Reviewed by NOBODY (OOPS!).
 4 
 5         Changed the check for URIs to only stop on null since empty URIs are valid css."

should be:

"3         Reviewed by NOBODY (OOPS!).
4 
5         Fix for: https://bugs.webkit.org/show_bug.cgi?id=17051
6         Changed the check for URIs to only stop on null since empty URIs are valid css."

[Bradley Meck]

Created attachment 21563 [details]
Last attempt to patch

[Eric Seidel (no email)]

Comment on attachment 21563 [details]
Last attempt to patch

Looks good to me.

[Eric Seidel (no email)]

Comment on attachment 21563 [details]
Last attempt to patch

Did you use svn-create-patch?  The patch still seems malformed...

[Eric Seidel (no email)]

Comment on attachment 21563 [details]
Last attempt to patch

HackTop [42186:WebKit]% curl "https://bugs.webkit.org/attachment.cgi?id=21563" | patch                                 [/Stuff/Projects/WebKit]
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5359  100  5359    0     0   7302      0 --:--:-- --:--:-- --:--:--  201k
patching file ChangeLog
patch: **** malformed patch at line 20: +        (WebCore::CSSParser::parseFillImage):

[Eric Seidel (no email)]

Ah, you must have edited the patch manually:

@@ -1,3 +1,14 @@

Says there should only be 13 lines... I think.

[Eric Seidel (no email)]

Comment on attachment 21563 [details]
Last attempt to patch

I can't get the patch to apply!  Perhaps we can work out over IRC what you're doing to generate a broken patch?

[Bradley Meck]

Created attachment 21567 [details]
Guess the new last attempt

[Darin Adler]

Comment on attachment 21567 [details]
Guess the new last attempt

Looks good. Layout test ChangeLog should have a comment explaining what the change is, but otherwise looks just right.

[Darin Adler]

When landing this I decided not to land the new test case, because what we really want is a text-only test rather than a test that dumps the render tree. I'm not even sure that the render tree dump shows the background color at all, so the test might not give different results if it fails (I should have just tried it but I didn't). Someone could decide later to land it if they like, but I decided not to land the test with only Mac results and without pixel results.

One of the four code changes is already covered by a test case (the cursor test) so I decided it was OK to land this without waiting for further test cases. It would be good to go back and add text-only tests for the others too.

Committed revision 34451.