647885 - getComputedStyle of background-position:0px 0px; returns "0% 0%", breaking Javascript CSS parsing

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[justin]

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.]

Attachment: testcase

Confirming

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.]

Opera 11.10 and Chrome 10 return 0px for "0%" and "0"
(Chrome returns 0px even for "0pc" or "0em")

Comment 4

[Boris Zbarsky [:bzbarsky]]

David, what do you think of doing that?

Comment 5

[j.j.]

> 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.]

IE9 supports getComputedStyle now. Checked attachment 524098 [details], returns "0px"
(using screenshot service http://meineipadresse.de/netrenderer/ )

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Keep better track of whether our computed background-position was specified with percentages.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Keep better track of whether our computed background-size was specified with percentages.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

> 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]]

Backed out these patches as mochitest-4 went orange: http://hg.mozilla.org/mozilla-central/rev/85d824b34830

Comment 14

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Patch 1 interdiff to fix the orange

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with interdiff applied

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 interdiff to fix the orange

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with interdiff applied

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Part 1 updated to comments

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to comments

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

http://hg.mozilla.org/mozilla-central/rev/6ccdd296b74b
http://hg.mozilla.org/mozilla-central/rev/2f37642dc741