Bug 25489 - background-position not shown
Summary: background-position not shown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Yaar Schnitman
URL:
Keywords:
Depends on:
Blocks: 26541
  Show dependency treegraph
 
Reported: 2009-04-30 12:47 PDT by Ash Searle
Modified: 2009-09-04 00:15 PDT (History)
3 users (show)

See Also:


Attachments
A Fix (4.35 KB, patch)
2009-08-24 16:09 PDT, Yaar Schnitman
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
fixes (3.40 KB, patch)
2009-09-03 13:46 PDT, Yaar Schnitman
no flags Details | Formatted Diff | Diff
Fixes this bug and 26541 as well (6.20 KB, patch)
2009-09-03 13:51 PDT, Yaar Schnitman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ash Searle 2009-04-30 12:47:23 PDT
The web inspector never shows background-position as part of the background shorthand property.  (Bug #20264 is probably a consequence of this.)

Minimal HTML to recreate:

<title>Web Inspector background-position test</title>
<style>p{background:url(#) no-repeat 2px -3px}</style>
<p>inspect this element</p>
Comment 1 Yaar Schnitman 2009-08-24 16:09:18 PDT
Created attachment 38503 [details]
A Fix

The patch depends on the patch for https://bugs.webkit.org/show_bug.cgi?id=26541

CSS property "background-position" was not serialized into the shorthand string value of "style.background". This is because position is stored into two (non-standard?) properties background-position-x and background-position-y. Since "style.backgroundPosition" does serialize as "position-x, position-y", (e.g. 50% 50%) it made sense that the shorthand value of style.background will do the same.

I fixed the layout test fast/dom/background-shorthand-csstext.html and fast/dom/background-shorthand-expected.txt as the later was accepting a FAIL.
Comment 2 Eric Seidel (no email) 2009-08-24 17:34:08 PDT
Comment on attachment 38503 [details]
A Fix

Do we have a specific section of the spec which talks about this?  Ideally we'd have a link to that section in the bug or the ChangeLog.
Comment 3 Eric Seidel (no email) 2009-08-24 17:34:44 PDT
Comment on attachment 38503 [details]
A Fix

Rejecting patch 38503 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=38503 from bug 25489 failed to download and apply.
Comment 4 Eric Seidel (no email) 2009-08-24 17:35:04 PDT
Comment on attachment 38503 [details]
A Fix

patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/background-shorthand-csstext-expected.txt
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/dom/background-shorthand-csstext-expected.txt.rej
patch -p0 "LayoutTests/fast/dom/background-shorthand-csstext-expected.txt" returned 1.  Pass --force to ignore patch failures.
Comment 5 Yaar Schnitman 2009-08-24 17:46:43 PDT
Comment on attachment 38503 [details]
A Fix

http://www.w3.org/TR/2008/WD-css3-background-20080910/#the-background-position
According to the spec's examples, background-position should indeed serialize as two numbers. In WebKit, these numbers are stored in background-position-x and background-position-y, but the user will never know. 

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index fa3b8ef..b8674a2 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,16 @@
> +2009-08-24  Yaar Schnitman  <yaar@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        This patch depends on the patch for bug #26541.
> +
> +	CSS property background-position was not serialized in shorthand string. The layout test accepted that failure, so I fixed them too.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=25489
> +
> +        * fast/dom/background-shorthand-csstext-expected.txt:
> +        * fast/dom/background-shorthand-csstext.html:
> +
>  2009-08-24  Gustavo Noronha Silva  <gustavo.noronha@collabora.co.uk>
>  
>          Unreviewed. Skip again tests that we enabled, since they are
> diff --git a/LayoutTests/fast/dom/background-shorthand-csstext-expected.txt b/LayoutTests/fast/dom/background-shorthand-csstext-expected.txt
> index cc4b3f2..13a79d1 100644
> --- a/LayoutTests/fast/dom/background-shorthand-csstext-expected.txt
> +++ b/LayoutTests/fast/dom/background-shorthand-csstext-expected.txt
> @@ -2,4 +2,4 @@ This page tests whether or not the background shorthand properly omits initial v
>  
>  PASS: document.body.style.background == 'green' should be true and is.
>  PASS: document.getElementById('div1').style.background == 'repeat-x, white repeat-y' should be true and is.
> -FAIL: document.getElementById('div2').style.background == '50% 50% blue' should be true but instead is false.
> +PASS: document.getElementById('div2').style.background == 'blue 50% 50%' should be true and is.
> diff --git a/LayoutTests/fast/dom/background-shorthand-csstext.html b/LayoutTests/fast/dom/background-shorthand-csstext.html
> index f633742..7f1d638 100644
> --- a/LayoutTests/fast/dom/background-shorthand-csstext.html
> +++ b/LayoutTests/fast/dom/background-shorthand-csstext.html
> @@ -33,7 +33,7 @@ function test()
>      
>      shouldBe("document.body.style.background == 'green'", true);
>      shouldBe("document.getElementById('div1').style.background == 'repeat-x, white repeat-y'", true);
> -    shouldBe("document.getElementById('div2').style.background == '50% 50% blue'", true);
> +    shouldBe("document.getElementById('div2').style.background == 'blue 50% 50%'", true);
>  }
>  </script>
>  </head>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index a42b457..cfc5b87 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> +2009-08-24  Yaar Schnitman  <yaar@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        This patch depends on the patch for bug #26541.
> +
> +	CSS property background-position was not serialized in shorthand string. The layout test accepted that failure, so I fixed them too.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=25489
> +     
> +        * css/CSSMutableStyleDeclaration.cpp:
> +        (WebCore::CSSMutableStyleDeclaration::getPropertyValue):
> +
>  2009-08-24  Nate Chapin  <japhet@chromium.org>
>  
>          Reviewed by Eric Seidel.
> diff --git a/WebCore/css/CSSMutableStyleDeclaration.cpp b/WebCore/css/CSSMutableStyleDeclaration.cpp
> index 85ff5e0..0d0235e 100644
> --- a/WebCore/css/CSSMutableStyleDeclaration.cpp
> +++ b/WebCore/css/CSSMutableStyleDeclaration.cpp
> @@ -115,10 +115,15 @@ String CSSMutableStyleDeclaration::getPropertyValue(int propertyID) const
>              return getLayeredShorthandValue(properties, 2);
>          }
>          case CSSPropertyBackground: {
> -            const int properties[7] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage, CSSPropertyBackgroundRepeat, 
> -                                        CSSPropertyBackgroundAttachment, CSSPropertyBackgroundPosition, CSSPropertyBackgroundClip,
> +            const int properties[8] = { CSSPropertyBackgroundColor, 
> +                                        CSSPropertyBackgroundImage, 
> +                                        CSSPropertyBackgroundRepeat, 
> +                                        CSSPropertyBackgroundAttachment, 
> +                                        CSSPropertyBackgroundPositionX,
> +                                        CSSPropertyBackgroundPositionY,
> +                                        CSSPropertyBackgroundClip,
>                                          CSSPropertyBackgroundOrigin };
> -            return getLayeredShorthandValue(properties, 7);
> +            return getLayeredShorthandValue(properties, 8);
>          }
>          case CSSPropertyBorder: {
>              const int properties[3][4] = {{ CSSPropertyBorderTopWidth,
Comment 6 Eric Seidel (no email) 2009-09-01 03:13:45 PDT
My understanding of the status of this bug is that the patch has rotted and thus cannot easily be landed as-is.  If someone could post an updated patch we can get this r+'d and cq+'d again.
Comment 7 Yaar Schnitman 2009-09-03 13:46:57 PDT
Created attachment 39006 [details]
fixes

 .../dom/background-shorthand-csstext-expected.txt  |    4 ++--
 .../fast/dom/background-shorthand-csstext.html     |    4 ++--
 WebCore/css/CSSMutableStyleDeclaration.cpp         |   14 ++++++++++----
 3 files changed, 14 insertions(+), 8 deletions(-)
Comment 8 Yaar Schnitman 2009-09-03 13:49:06 PDT
Comment on attachment 39006 [details]
fixes

Ignore. git-send-bugzilla failed to upload this right
Comment 9 Yaar Schnitman 2009-09-03 13:51:21 PDT
Created attachment 39007 [details]
Fixes this bug and 26541 as well
Comment 10 Eric Seidel (no email) 2009-09-03 15:48:12 PDT
FYI, "bugzilla-tool post-commits" is a drop-in replacement for git-send-bugzilla.  Two things to note:
1.  post-commits marks review by default, pass --no-review to disable
2.  post-commits cherry-picks by default, if you want to attach a range of patches you have to specify it.  "HEAD~3" will just attach one patch, not 3. :)
Comment 11 Eric Seidel (no email) 2009-09-03 15:50:16 PDT
Comment on attachment 39007 [details]
Fixes this bug and 26541 as well

LGTM.
Comment 12 Eric Seidel (no email) 2009-09-03 16:01:30 PDT
Comment on attachment 39007 [details]
Fixes this bug and 26541 as well

Rejecting patch 39007 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None
Comment 13 Yaar Schnitman 2009-09-03 17:03:15 PDT
What's wrong with the patch?
Comment 14 Eric Seidel (no email) 2009-09-03 23:56:09 PDT
Comment on attachment 39007 [details]
Fixes this bug and 26541 as well

Race condition during commit.  ChangeLog was out of date by the time it went to commit.  Trying again.
Comment 15 Eric Seidel (no email) 2009-09-04 00:15:43 PDT
Comment on attachment 39007 [details]
Fixes this bug and 26541 as well

Clearing flags on attachment: 39007

Committed r48040: <http://trac.webkit.org/changeset/48040>
Comment 16 Eric Seidel (no email) 2009-09-04 00:15:48 PDT
All reviewed patches have been landed.  Closing bug.