84599 - getComputedStyle() returns incorrect values for position properties

Status: RESOLVED FIXED

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

Description

[tfriesen]

Problem #1

Depending on the 2nd argument,getComputedStyle seems to measure from the 
element's border edge or padding edge.
Its my understanding that computed values are derived from the specified values.
The style properties measure from the margin edge and I think the computed
values should reflect this.

Problem #2

The right and bottom are calculated from the left/top of the parent instead
of the right/bottom,this is incorrect behavior according to CSS2 specs

Comment 1

[tfriesen]

Attachment: select and click div to get values

Comment 2

[Johnny Stenback (:jst)]

Harish, could you look into this, especially problem #2?

Comment 3

[tfriesen]

To clarify problem # 2
The correct way to measure for the right property (according to the specs),would 
be to go from the right edge of the child to the right edge of the parent.
getComputedStyle() measures from the parent's left edge to the child's right 
edge.
The same kinda thing happens with the bottom property.

Comment 4

[harishd]

--> 0.9.5

Comment 5

[harishd]

Moving back to 0.9.4.

Comment 6

[harishd]

ain't going to make it to 0.9.4 train. Moving to 0.9.5.

Comment 7

[harishd]

I do have a fix for the right property but haven't gotten the time to test it :-(
Will look into it at the earliest possible.

Comment 8

[harishd]

Attachment: partial fix..

Comment 9

[harishd]

Handing over getComputedStyle bugs to bzbarsky. Thanks boris.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Problem #1 is bug 73525.

Working on #2.

Comment 11

[tfriesen]

To clarify problem #1

To get the computed top value of an abs. pos. element,you would
measure from the element's top margin edge to the top padding edge of
the offsetParent (assuming it is block level).

getComputedStyle() does not measure from the element's margin edge,
it measures from the border or padding edge (depending on the 2nd argument).

It should measure from the margin edge of the child.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Javascript for testcases

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: CSS for testcases

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple abs. pos. testcase

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple rel. pos. testcase

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple fixed pos. testcase

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Static position testcase

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Abs. pos. in rel. pos. testcase

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Rel. pos. in rel. pos. testcase

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: rel. pos. using percentages testcase

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: abs. pos. using percentages testcase

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix most of this stuff

Comment 23

[Boris Zbarsky [:bzbarsky]]

The attached patch fixes positioning properties for all cases I could think of
_except_:

1)  left/right/top/bottom for statically positioned elements (returns
    NS_ERROR_NOT_IMPLEMENTED at the moment)
2)  width/height for non-replaced inline elements (returns
    NS_ERROR_NOT_IMPLEMENTED at the moment)

Tested on the attached testcases as well as using DOM inspector on
http://web.mit.edu/bzbarsky/www/books.html (for a static positioning with
scrollbar testcase).

reviews?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 55568 [details] [diff] [review]
Patch to fix most of this stuff

Forgot a null-check

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: ignore things with display:none (we don't compute for them anyway)

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Compute all the properties we can for display:none and _still_ not crash. :)

Comment 27

[Boris Zbarsky [:bzbarsky]]

glazou, would you care to take a look?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Doh. Missed a null-check in GetZIndex()

Comment 29

[tfriesen]

Boris,

In your test case for static positioning,you show that some properties should
return values in pixels and some in different units.
Is getPropertyValue() suppose to sniff out the the unit used in the styling
and return the appropriate value?
In the past these values were only returned in pixels. 

Comment 30

[Boris Zbarsky [:bzbarsky]]

Well... The static position case is odd because those properties get ignored
(and hence not computed) for statically positioned elements.  Depending on who
you ask, we're supposed to return any of:

1) 0
2) The value set in the sheet converted into pixels
3) The value set in the sheet converted into pixels unless it's a percentage
   value, in which case we should return the percentage value (eg "15%")
4) The value set in the sheet with no modifications

My testcase was written to cover case 4, which is the hardest of the set to
implement.  If you note, I actually make those properties throw an exception on
getting, pending decision on what should be done with them.

Any useful info on that would be much appreciated.

Comment 31

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

just one thing : nsComputedDOMStyle::GetContainingBlock's name should be changed
since it looks for the first containing frame which does not have %age-based size.
Continuing the review.

Comment 32

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 55999 [details] [diff] [review]
Doh.  Missed a null-check in GetZIndex()

>Index: content/shared/src/nsStyleStruct.cpp
>...
>+            if (borderData) {
>+              nsMargin border;
>+                borderData->CalcBorderFor(frame, border);
>+                baseWidth -= (border.left + border.right);
>+            }

only indentation.

>+  nsIFrame* GetContainingBlock(nsIFrame *aFrame);

I wonder if nsComputedDOMStyle is really the place for this and if it
could not be shared with other pieces of code.

Looks fine otherwise ! r=glazman

Comment 33

[Boris Zbarsky [:bzbarsky]]

> since it looks for the first containing frame which does not have %age-based
> size.

Actually, it does not.  It looks for the first containing frame that is
positioned or a block. The magic comes from the frame hierarchy -- positioned
elements get reparented to their containing block (hence placeholder frames). 
And yes, this function _does_ duplicate existing reflow functionality, as does
half this code.  That's the subject of bug 106154.

Indentation fixed.  Thanks for the review, Daniel!

Comment 34

[Boris Zbarsky [:bzbarsky]]

Didn't make it.  pushing out.

Comment 35

[Marc Attinasi]

Comment on attachment 55999 [details] [diff] [review]
Doh.  Missed a null-check in GetZIndex()

sr=attinasi

Comment 36

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the reviews!