Last Comment Bug 647885 - getComputedStyle of background-position:0px 0px; returns "0% 0%", breaking Javascript CSS parsing
: getComputedStyle of background-position:0px 0px; returns "0% 0%", breaking Ja...
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla6
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 657905 (view as bug list)
Depends on:
Blocks: 594934 1063815
  Show dependency treegraph
 
Reported: 2011-04-05 13:33 PDT by justin
Modified: 2014-09-05 14:58 PDT (History)
7 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (224 bytes, text/html)
2011-04-05 15:28 PDT, j.j.
no flags Details
part 1. Keep better track of whether our computed background-position was specified with percentages. (8.23 KB, patch)
2011-05-19 11:14 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
part 2. Keep better track of whether our computed background-size was specified with percentages. (8.50 KB, patch)
2011-05-19 11:17 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
Patch 1 interdiff to fix the orange (4.72 KB, patch)
2011-05-23 18:58 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
Part 1 with interdiff applied (12.89 KB, patch)
2011-05-23 19:00 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 2 interdiff to fix the orange (3.72 KB, patch)
2011-05-23 19:01 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
Part 2 with interdiff applied (12.05 KB, patch)
2011-05-23 19:02 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 1 updated to comments (12.88 KB, patch)
2011-05-23 20:45 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 2 updated to comments (14.33 KB, patch)
2011-05-23 20:45 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description justin 2011-04-05 13:33:44 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0

My web site that had worked in all other browsers, had Javascript errors in Firefox 4, because of a difference in how "background-position" values were read from an html element. One of my elements had a background-position of 0px 0px, which FF4 converted to 0% 0%. I was parsing the css property with Javascript, and ONLY in FF4 did my String.split() on "px" break, because the browser converted those values. I believe this behavior to be non-standard.

Reproducible: Always

Steps to Reproduce:
1. Create an HTML element with background-position:0px 0px
2. Use Javascript to grab the element.style.backgroundPosition value
3. NOTE: I used Prototype.js to grab the css value: element.getStyle('background-position');
4. split() the value on "px", and you don't get the value that every other browser gives you. 
5. example of split to get vertical background-position: parseInt( bgPosition.split('px ')[1].split('px')[0] );

Actual Results:  
FF4 gives you NaN, because there was no longer a "px" to split on.

Expected Results:  
Other browsers give you 0 (zero), which was the correct, because javascript corrently read "0px 0px" from the element's css 

This happens on both my Windows XP and OS X machines in FF4.

For a quick fix, I had to replace the "%" that FF4 added, with the "px" that my code was expecting. 

example: 

var bgPosition = element.getStyle('background-position');
bgPosition.replace('0%','0px');	// FF4 fix
var bgYOffset = parseInt( bgPosition.split('px ')[1].split('px')[0] );
// bgYOffset now has correct background y-position.
Comment 1 j.j. 2011-04-05 15:28:39 PDT
Created attachment 524098 [details]
testcase

Confirming
Comment 2 Boris Zbarsky [:bz] 2011-04-05 15:50:44 PDT
This is a behavior change from bug 594934.

The logic in DoGetBackgroundPosition looks like this:

1533     if (pos.mXPosition.mLength == 0) {
1534       valX->SetPercent(pos.mXPosition.mPercent);
1535     } else if (pos.mXPosition.mPercent == 0.0f) {
1536       valX->SetAppUnits(pos.mXPosition.mLength);
1537     } else {
1538       nsStyleCoord::Calc calc;
1539       calc.mPercent = pos.mXPosition.mPercent;
1540       calc.mLength  = pos.mXPosition.mLength;
1541       calc.mHasPercent = PR_TRUE;
1542       SetValueToCalc(&calc, valX);
1543     }

The problem is that by the time we get here we no longer know whether the position was specified as 0px or 0%; they're stored identically in the computed style...  What do other browsers return from computed style when you use 0%?

(I should note that the code in comment 0 is all kinds of broken in the face of user stylesheets, but let's ignore that for now.)
Comment 3 j.j. 2011-04-05 16:26:37 PDT
Opera 11.10 and Chrome 10 return 0px for "0%" and "0"
(Chrome returns 0px even for "0pc" or "0em")
Comment 4 Boris Zbarsky [:bz] 2011-04-05 16:31:08 PDT
David, what do you think of doing that?
Comment 5 j.j. 2011-04-05 16:38:07 PDT
> Opera 11.10 and Chrome 10 return 0px for "0%" and "0"
Ignore that!

Opera 11.10 and Chrome return 0% for "0%" and 0px for plain "0"
Comment 6 j.j. 2011-04-05 16:51:45 PDT
IE9 supports getComputedStyle now. Checked attachment 524098 [details], returns "0px"
(using screenshot service http://meineipadresse.de/netrenderer/ )
Comment 7 Boris Zbarsky [:bz] 2011-05-18 23:52:41 PDT
*** Bug 657905 has been marked as a duplicate of this bug. ***
Comment 8 Boris Zbarsky [:bz] 2011-05-19 11:14:48 PDT
Created attachment 533712 [details] [diff] [review]
part 1.  Keep better track of whether our computed background-position was specified with percentages.
Comment 9 Boris Zbarsky [:bz] 2011-05-19 11:17:09 PDT
Created attachment 533714 [details] [diff] [review]
part 2.  Keep better track of whether our computed background-size was specified with percentages.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-21 10:27:25 PDT
Comment on attachment 533712 [details] [diff] [review]
part 1.  Keep better track of whether our computed background-position was specified with percentages.

mHasPercent is a PRPackedBool, so really you should be using PR_TRUE/PR_FALSE rather than true/false.

Shouldn't nsStyleBackground::Position::SetInitialValues initialize mHasPercent to PR_TRUE, since the initial value in the spec is "0% 0%"?  Add a test?

Also, drop the new blank line in nsRuleNode in the calc() case.

r=dbaron with that
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-21 10:35:52 PDT
Comment on attachment 533714 [details] [diff] [review]
part 2.  Keep better track of whether our computed background-size was specified with percentages.

r=dbaron with PR_TRUE/PR_FALSE in the nsRuleNode.cpp parts (rather than true/false)
Comment 12 Boris Zbarsky [:bz] 2011-05-23 13:30:30 PDT
> mHasPercent is a PRPackedBool, so really you should be using PR_TRUE/PR_FALSE

Done.

> Shouldn't nsStyleBackground::Position::SetInitialValues initialize mHasPercent
> to PR_TRUE

Yes, good catch.  Done, and test added.

> Also, drop the new blank line in nsRuleNode in the calc() case.

Done.
Comment 13 Dave Townsend [:mossop] 2011-05-23 15:58:41 PDT
Backed out these patches as mochitest-4 went orange: http://hg.mozilla.org/mozilla-central/rev/85d824b34830
Comment 14 Boris Zbarsky [:bz] 2011-05-23 17:53:18 PDT
Looks like there were two failures:

1)  "0px 0px" is not an initial value of background-position.  This was a bug in
    the test.
2)  nsStyleAnimation::ExtractComputedValue didn't get changed to mirror the
    nsComputedDOMStyle changes.
Comment 15 Boris Zbarsky [:bz] 2011-05-23 18:58:56 PDT
Created attachment 534652 [details] [diff] [review]
Patch 1 interdiff to fix the orange
Comment 16 Boris Zbarsky [:bz] 2011-05-23 19:00:06 PDT
Created attachment 534654 [details] [diff] [review]
Part 1 with interdiff applied
Comment 17 Boris Zbarsky [:bz] 2011-05-23 19:01:21 PDT
Created attachment 534655 [details] [diff] [review]
Part 2 interdiff to fix the orange
Comment 18 Boris Zbarsky [:bz] 2011-05-23 19:02:27 PDT
Created attachment 534656 [details] [diff] [review]
Part 2 with interdiff applied
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-23 19:51:30 PDT
Comment on attachment 534652 [details] [diff] [review]
Patch 1 interdiff to fix the orange

>+            if (pos.mYPosition.mPercent == 0.0f) {

Here you meant if (!pos.mYPosition.mHasPercent) {

r=dbaron with that

(Can you add a test with +0% that catches the difference... might be hard?)
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-23 19:53:34 PDT
Comment on attachment 534655 [details] [diff] [review]
Part 2 interdiff to fix the orange

Maybe it would be good to add the assertion about percent being 0 in the !mHasPercent cases here too?

r=dbaron
Comment 21 Boris Zbarsky [:bz] 2011-05-23 20:18:16 PDT
> Can you add a test with +0% that catches the difference.

Sure.  Interpolating from "40% 0%" to "0% 0%" lands at "30% 0px" when 1/4 of the way through due to this issue; easy to test.  I'll add a test and fix the code.

> Maybe it would be good to add the assertion about percent being 0 in the
> !mHasPercent cases here too?

Will do.

To be clear, the reason I didn't just use SetCalcValue is that I wasn't sure why we weren't doing it before....
Comment 22 Boris Zbarsky [:bz] 2011-05-23 20:45:12 PDT
Created attachment 534665 [details] [diff] [review]
Part 1 updated to comments
Comment 23 Boris Zbarsky [:bz] 2011-05-23 20:45:38 PDT
Created attachment 534666 [details] [diff] [review]
Part 2 updated to comments
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-23 23:37:38 PDT
http://hg.mozilla.org/mozilla-central/rev/6ccdd296b74b
http://hg.mozilla.org/mozilla-central/rev/2f37642dc741

Note You need to log in before you can comment on or make changes to this bug.