Bug 25489 – background-position not shown

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ash Searle]

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>

[Yaar Schnitman]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Yaar Schnitman]

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 @@
> [phone]  Yaar Schnitman  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Yaar Schnitman  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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,

[Eric Seidel (no email)]

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.

[Yaar Schnitman]

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(-)

[Yaar Schnitman]

Comment on attachment 39006 [details]
fixes

Ignore. git-send-bugzilla failed to upload this right

[Yaar Schnitman]

Created attachment 39007 [details]
Fixes this bug and 26541 as well

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

Comment on attachment 39007 [details]
Fixes this bug and 26541 as well

LGTM.

[Eric Seidel (no email)]

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

[Yaar Schnitman]

What's wrong with the patch?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.