The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
DOM: CSS Object Model
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: justin, Assigned: bz)

Tracking

({regression, testcase})

Trunk
mozilla6
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

224 bytes, text/html
Details
12.88 KB, patch
Details | Diff | Splinter Review
14.33 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Version: unspecified → 4.0 Branch

Comment 1

6 years ago
Created attachment 524098 [details]
testcase

Confirming

Updated

6 years ago
Status: UNCONFIRMED → NEW
Component: General → General
Ever confirmed: true
Keywords: regression, testcase
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → general
Hardware: x86 → All
Summary: A background-position of "0px 0px" gets translated to "0% 0%", breaking Javascript CSS parsing → getComputedStyle of background-position:0px 0px; returns "0% 0%", breaking Javascript CSS parsing
Version: 4.0 Branch → 2.0 Branch

Updated

6 years ago
Component: General → DOM: CSS Object Model
QA Contact: general → style-system
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.)
Blocks: 594934

Comment 3

6 years ago
Opera 11.10 and Chrome 10 return 0px for "0%" and "0"
(Chrome returns 0px even for "0pc" or "0em")
David, what do you think of doing that?

Comment 5

6 years ago
> 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

6 years ago
IE9 supports getComputedStyle now. Checked attachment 524098 [details], returns "0px"
(using screenshot service http://meineipadresse.de/netrenderer/ )
Duplicate of this bug: 657905
Created attachment 533712 [details] [diff] [review]
part 1.  Keep better track of whether our computed background-position was specified with percentages.
Attachment #533712 - Flags: review?(dbaron)
Created attachment 533714 [details] [diff] [review]
part 2.  Keep better track of whether our computed background-size was specified with percentages.
Attachment #533714 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Version: 2.0 Branch → Trunk
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
Attachment #533712 - Flags: review?(dbaron) → review+
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)
Attachment #533714 - Flags: review?(dbaron) → review+
> 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.
Backed out these patches as mochitest-4 went orange: http://hg.mozilla.org/mozilla-central/rev/85d824b34830
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.
Created attachment 534652 [details] [diff] [review]
Patch 1 interdiff to fix the orange
Attachment #534652 - Flags: review?(dbaron)
Created attachment 534654 [details] [diff] [review]
Part 1 with interdiff applied
Created attachment 534655 [details] [diff] [review]
Part 2 interdiff to fix the orange
Attachment #534655 - Flags: review?(dbaron)
Created attachment 534656 [details] [diff] [review]
Part 2 with interdiff applied
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?)
Attachment #534652 - Flags: review?(dbaron) → review+
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
Attachment #534655 - Flags: review?(dbaron) → review+
> 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....
Created attachment 534665 [details] [diff] [review]
Part 1 updated to comments
Attachment #534654 - Attachment is obsolete: true
Created attachment 534666 [details] [diff] [review]
Part 2 updated to comments
Attachment #534656 - Attachment is obsolete: true
Attachment #533712 - Attachment is obsolete: true
Attachment #533714 - Attachment is obsolete: true
Attachment #534652 - Attachment is obsolete: true
Attachment #534655 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/6ccdd296b74b
http://hg.mozilla.org/mozilla-central/rev/2f37642dc741
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [need review]
Target Milestone: --- → mozilla6
Blocks: 1063815
You need to log in before you can comment on or make changes to this bug.