Closed
Bug 84599
Opened 23 years ago
Closed 23 years ago
getComputedStyle() returns incorrect values for position properties
Categories
(Core :: DOM: CSS Object Model, defect, P1)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
FIXED
mozilla0.9.7
People
(Reporter: tfriesen, Assigned: bzbarsky)
References
Details
Attachments
(13 files, 3 obsolete files)
772 bytes,
text/html
|
Details | |
1.45 KB,
patch
|
Details | Diff | Splinter Review | |
2.87 KB,
application/x-javascript
|
Details | |
675 bytes,
text/css
|
Details | |
1.15 KB,
text/html
|
Details | |
1.18 KB,
text/html
|
Details | |
1.17 KB,
text/html
|
Details | |
1.79 KB,
text/html
|
Details | |
1.26 KB,
text/html
|
Details | |
1.31 KB,
text/html
|
Details | |
1.47 KB,
text/html
|
Details | |
1.32 KB,
text/html
|
Details | |
29.90 KB,
patch
|
glazou
:
review+
attinasi
:
superreview+
|
Details | Diff | Splinter Review |
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 2•23 years ago
|
||
Harish, could you look into this, especially problem #2?
Assignee: jst → harishd
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.
Assignee | ||
Updated•23 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 95 → All
Hardware: PC → All
--> 0.9.5
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
ain't going to make it to 0.9.4 train. Moving to 0.9.5.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
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.
Handing over getComputedStyle bugs to bzbarsky. Thanks boris.
Assignee: harishd → bzbarsky
Status: ASSIGNED → NEW
Assignee | ||
Comment 10•23 years ago
|
||
Problem #1 is bug 73525. Working on #2.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Reporter | ||
Comment 11•23 years ago
|
||
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.
Assignee | ||
Comment 12•23 years ago
|
||
Assignee | ||
Comment 13•23 years ago
|
||
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Comment 17•23 years ago
|
||
Assignee | ||
Comment 18•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #55562 -
Attachment description: abs. pos. using percentages testcase → rel. pos. using percentages testcase
Assignee | ||
Comment 21•23 years ago
|
||
Assignee | ||
Comment 22•23 years ago
|
||
Assignee | ||
Comment 23•23 years ago
|
||
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?
Assignee | ||
Comment 24•23 years ago
|
||
Comment on attachment 55568 [details] [diff] [review] Patch to fix most of this stuff Forgot a null-check
Attachment #55568 -
Attachment is obsolete: true
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #55585 -
Attachment is obsolete: true
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
glazou, would you care to take a look?
Assignee | ||
Updated•23 years ago
|
Attachment #55612 -
Attachment is obsolete: true
Assignee | ||
Comment 28•23 years ago
|
||
Reporter | ||
Comment 29•23 years ago
|
||
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.
Assignee | ||
Comment 30•23 years ago
|
||
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.
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 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
Attachment #55999 -
Flags: review+
Assignee | ||
Comment 33•23 years ago
|
||
> 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!
Assignee | ||
Comment 34•23 years ago
|
||
Didn't make it. pushing out.
Comment 35•23 years ago
|
||
Comment on attachment 55999 [details] [diff] [review] Doh. Missed a null-check in GetZIndex() sr=attinasi
Attachment #55999 -
Flags: superreview+
Assignee | ||
Comment 36•23 years ago
|
||
Checked in. Thanks for the reviews!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•