Closed
Bug 174397
Opened 21 years ago
Closed 16 years ago
Support for getClientRects and getBoundingClientRect
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: erik, Assigned: roc)
References
Details
(Keywords: dev-doc-complete, testcase)
Attachments
(4 files, 4 obsolete files)
http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/getclientrects.asp http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/getboundingclientrect.asp These two methods return a TextRectangle object which describes the size and position of the element relative to the client area. Currently the bounding rectangle can be found but it is not very straight forward and it is very error prone. This information is already available inside the layout engine and it would make sense to provide interfaces for these to scripting. The client rects for inline elements that wrap is today not available to scripting
![]() |
||
Comment 1•21 years ago
|
||
Over to the right component. This _could_ be done, except for one little problem -- the MSDN descriptions are so vague as to be useless. Do these rects include margins? Borders? Padding? How does this interact with positioning? With floating? What are the correct values for table elements in the collapsing border table model? Without either documentation or a clear and exhaustive series of testcases which access the property and say what it _should_ be (what IE returns) in addition to having the test for what it _is_, it's a little hard to do any real work on this bug.
Component: DOM Views and Formatting → DOM Mozilla Extensions
OS: Windows XP → All
QA Contact: stummala → lchiang
Hardware: PC → All
Reporter | ||
Comment 2•21 years ago
|
||
The rect used is the border-box relative to the client viewport I'll be willing to create the needed test cases.
Reporter | ||
Comment 3•21 years ago
|
||
This test case when run in IE places an element over the target (element, not text nodes). Notice that to do this in Mozilla would require loops to sum up offsetsLeft, scrollLeft, paddingLeft, BorderLeftWidth for all ancestors. Not very efficient and very error prone. The red dotted border is for the getBoundingClientRect and the black dottet borders are for getClientRects. The client rects information is not available in Mozilla. Implementing getBoundingClientRect should be trivial. The other one is a bit trickier.
![]() |
||
Comment 4•21 years ago
|
||
Actually, the other is likely a lot more trivial... How should this interact with -moz-box-sizing? Or does that not matter? How does it interact with overflowing content (eg offsetWidth in IE _includes_ the overflowing elements, whereas the CSS border-box most obviously does not).
Reporter | ||
Comment 5•21 years ago
|
||
-moz-box-sizing does not pose a problem. The BoundingClientRect is always the outer border box. The overflow issue in the case of overflow being set to visible is of course a problem, just like it is a problem for offsetWith/Height. I think you should be consistent and use the same logic here as well. That is to ignore the overflown content and use the outer border edge. For all elements the following should be true: el.getBoundingClientRect().right - el.getBoundingClientRect().left = el.offsetWidth el.getBoundingClientRect().bottom - el.getBoundingClientRect().top = el.offsetHeight I'll attach another test case that shows that this is always true. Notice also that getBoundingClientRect can be calcualted directly from getClientRects by taking the min/max values for all the rects. This will also be shown in the upcoming attachement.
Reporter | ||
Comment 6•21 years ago
|
||
Resize the browser window so that the text with red background wraps. Click on elements to alert offsetWidth/Height and getBoundingClientRect This test case shows that getBoundingClientRect can be calculated from getClientRects. It also shows the relationship between getBoundingClientRect and offsetWidth/Height. (it also shows a but in Mozilla implementation of offsetWidth/Height... I'll see if I can find the related bug or file a new one.)
Reporter | ||
Comment 7•21 years ago
|
||
I just found out about document.getBoxObjectFor(oElement). This seems to allow me to find the position of an element without relying on recursive functions using offset* and scroll*. This practically removes the need for getBoundingClientRect.
Comment 8•21 years ago
|
||
Hi, Just tested document.getBoxObjectFor(oElement), and it doesn't work for #text nodes. We need to get the rects for a selection of text that my be wrapping inside a middle of <P> element (for example). That's what getBoundingClientRect does for us..
Comment 9•21 years ago
|
||
Oops, I meant getClientRects() get the rects for wrapped text, in last post.
![]() |
||
Comment 10•21 years ago
|
||
I wouldn't rely on getBoxObjectFor too much. It's designed for XUL; its interaction with the CSS box model is rather undefined. And it may be getting moved to XULDocument, in which case it would just not be available in HTML.
Reporter | ||
Comment 11•21 years ago
|
||
That was what I was afraid of. These properties are needed in all Documents with a view, not only some certain flavors of XML. I can see a few possible ways to go: 1. Keep getBoxObjectFor for all Documents (or make boxObject a property of all Elements and not only XULElements) 2. Implement Element getBoundingClientRect. Maybe this is better because it will make it compatible with IE and will hide the other methods and properties visible in the BoxObject interface which may or may not be desired to be public in all kinds of Document views. 3. Introduce something entirely new that is part of the view. I thought this was the initial intention for the W3C DOM Views but that part of the DOM has totally failed. Maybe in level 4? But the world cannot wait that long.
Keywords: testcase
Comment 13•19 years ago
|
||
"But the world cannot wait that long." Ironic, as that was posted 23 months ago. Is there a specific reason that this functionality can't be added in? The comments seem inconclusive. I think this bug deserves a revisitation.
![]() |
||
Comment 14•19 years ago
|
||
> Is there a specific reason that this functionality can't be added in?
Lack of a clear concept of what we want exactly, coupled with lack of time to
clearly spec this out and then implement it.
If someone has time to work on this, they should go for it.
Keywords: helpwanted
Reporter | ||
Comment 15•19 years ago
|
||
(In reply to comment #13) > "But the world cannot wait that long." > > Ironic, as that was posted 23 months ago. Is there a specific reason that this > functionality can't be added in? The comments seem inconclusive. I think this > bug deserves a revisitation. I agree that it is ironic. During this time document.getBoxObjectFor has become the defacto way for doing this in Mozilla (or recursive solutions that always seem to fail in some cases). So I guess the world did not stop dead ;-)
Comment 16•18 years ago
|
||
Any chance of this bug being fixed for Gecko 1.8?
![]() |
||
Comment 17•18 years ago
|
||
Darin, see comment 1. Most of what I said in it still stands -- if someone can clearly describe what needs to be implemented we could implement it, but I, personally, am not willing to implement something different from IE and then spend forever fixing it up one hack at a time...
Comment 18•18 years ago
|
||
bz: that sounds reasonable.
Comment 19•18 years ago
|
||
*** Bug 323078 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 20•17 years ago
|
||
My latest thinking on this, after chatting with one of the Mochikit people, is that implementing getBoundingClientRect is not a good idea. The IE implementation has some nasty quirks which we don't want to emulate (as well as being poorly specified), and if we're not going to emulate them, we might as well call our API something else. Hopefully this person is going to propose an API with a solid spec to WHATWG; then we can implement that.
Comment 21•17 years ago
|
||
I can fix "poorly specified"; can you expand on "nasty quirks"?
Assignee | ||
Comment 22•17 years ago
|
||
http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/getboundingclientrect.asp "In Microsoft Internet Explorer 5, the window's upper-left is at 2,2 (pixels) with respect to the true client." I'm not even sure what that means, but it can't be good :-)
Comment 23•17 years ago
|
||
If that's it, then that's not enough to invent a whole new API, IMHO.
Assignee | ||
Comment 24•17 years ago
|
||
Beau, is the mysterious 2px offset the only problem, or are there other problems? I got the impression there were other problems, but I could have the wrong impression.
Assignee | ||
Comment 25•17 years ago
|
||
Note, apparently whether there is a 2px offset or not depends on quirks vs standards mode.
Comment 26•17 years ago
|
||
That's even better, it means we can implement it without feeling bad. :-)
Comment 27•17 years ago
|
||
IE inserts the (2,2) offset in standards mode, not quirks mode. It's easy to work around. They set document.documentElement.clientLeft and document.documentElement.clientTop to 2 in standards mode, or 0 in quirks mode. From limited testing, IE's getBoundingClientRect() seems to return the same values as a recursive offsetLeft + clientLeft summation. They even share a bug where the left padding is included in a calculation, but the top padding is not. IE's offsetLeft/offsetTop are a real mess of hasLayout and who knows what else. If getBoundingClientRect() is just a different interface to the same mess, I don't see how trying to clone that in Mozilla will help. IE, Mozilla, WebKit, and Opera return different summations for an object positioned on a test page I built (and will attach). I think Mozilla should implement something new and correct, and leave it to the JS libraries to work around IE's bugs. Either way, let's just get this right.
Comment 28•17 years ago
|
||
Comment 29•17 years ago
|
||
The problem with anything "new and correct" is that it would be just as underdefined unless someone actually defined it, and defining it is hard. We're going to have to define offsetTop/getClientRects anyway, so we might as well define them sensibly and implement them the same in all modern browsers, instead of inventing yet another set of methods. Anne has worked on defining offsetTop, etc, maybe he knows how it should work in enough detail to spec it?
Assignee | ||
Comment 30•17 years ago
|
||
The version of Anne's spec that I saw was very complicated, as it tried to capture a lot of quirks. If we define offset* "sensibly", then it won't be anything like what any browser currently implements, so why give it the same name and break people who are currently using it (or force them to add browser version tests instead of just a feature test).
Comment 31•17 years ago
|
||
I would imagine that anything we implement would be very complicated, even without quirks.
Assignee | ||
Comment 32•17 years ago
|
||
http://dump.testsuite.org/2006/dom/style/offset/spec has a spec proposal for the offset* properties (written by Anne among others). [A few more issues that need to be addressed there: -- what about elements that generate multiple CSS boxes (e.g. broken inlines)? -- overflow can affect offset*, because overflow affects whether scrollbars are present and hence layout; the text needs to be clarified -- table collapsed borders, are the cells treated as having overlapping border boxes? -- what about SVG and content inside a transformed (rotated) foreignobject?] In comment #30 I jumped the gun a bit. If we define the "client rects" of an element in terms of the sum of the offset* properties (reinterpreted as to apply to each of the element's CSS boxes) from the element all the way up the offsetParent chain, then a lot of the offset* quirks become irrelevant because it doesn't really matter how offsetParent is chosen. In fact I think it would work quite well except for one thing: elements with a null offsetParent have their offsetX/Y relative to the canvas origin if they are fixed-position, but the initial containing block origin otherwise. This means that the client rect(s) for an element would have a different reference point depending on whether the element is in a fixed-pos container or not, which would be very bad. Perhaps we could fix that by having the offset* spec simply treat position:fixed the same as other positions and returned offsets relative to the initial containing block origin. position:fixed isn't used much on the Web yet.
Comment 33•17 years ago
|
||
The initial containing block is defined as being a rectangle with the dimensions of the viewport, anchored at the canvas origin. So there should be no difference there (maybe apart from the 2,2 weirdness mentioned earlier).
Assignee | ||
Comment 34•17 years ago
|
||
The 2,2 weirdness is simply inconsistent with this spec. I'm happy to ignore it :-). Assuming you're right, then that spec should be amended to make it clear there is no difference. Then defining getBoundingClientRect in terms of the offset* properties basically amounts to "the offset of the top-left of the border-box from the origin of the canvas, assuming everything scrolled to the default position". Which is what I was aiming for all along :-).
Comment 35•17 years ago
|
||
My specification doesn't try to capture many quirks actually. If it did, it would require changes to the CSS box model and would require browsers to implement hasLayout. I think it's more sane than Mozilla's current implementation actually. The part after "Notes" is to be ignored. FWIW, that specification is here: http://dump.testsuite.org/2006/dom/style/offset/spec If you want the reason for various design choices, feel free to ask. It would be great to get this the same between Opera, Mozilla and Safari (and others).
Assignee | ||
Comment 36•17 years ago
|
||
What about comment #33?
Comment 37•17 years ago
|
||
(In reply to comment #32) > -- what about elements that generate multiple CSS boxes (e.g. broken inlines)? Haven't gotten around to that yet. I'm open to suggestions. > -- overflow can affect offset*, because overflow affects whether scrollbars > are present and hence layout; the text needs to be clarified I should probably state this differently, but the intend was to say what you said in comment 34. > -- table collapsed borders, are the cells treated as having overlapping > border boxes? Open to suggestions. Might also be good to test IE for this. (Can't really test that at the moment.) > -- what about SVG and content inside a transformed (rotated) foreignobject?] SVG elements don't implement HTMLElement so they don't have to work wit this model. I guess we could move the properties to Element instead and define it for all possible layout cases... Not sure about transformed content. I think comment 33 can be implemented as stated given that you don't take scrolling into account ("everything scrolled to its default position," as you said). Will make these modifications either this weekend or early next week.
Updated•17 years ago
|
Attachment #224960 -
Attachment is obsolete: true
Comment 38•17 years ago
|
||
This test shows which dimensions are included in Mozilla’s getBoxObjectFor(), IE’s getBoundingClientRect(), and by recursively summing offset and (client || 0). getBoundingClientRect() seems to include different measurements than the summation. (div#one’s margin is counted in offset *and* client.)
Comment 39•17 years ago
|
||
Clarified the specification a bit with regard to some of the comments. For inline elements that generate multiple boxes you take the first box and calculate from there. That seems to be what browsers do at the moment.
Assignee | ||
Comment 40•17 years ago
|
||
Is that "first" in content order or visual order (for bidi-reordered content)? The question about SVG is, if I have a situation where an SVG foreignobject element contains an HTML element, and I call offsetX and offsetY on that HTML element, what are the results?
Comment 41•17 years ago
|
||
I tried to make a test using RTL... http://dump.testsuite.org/2006/dom/style/offset/049.htm Browser offsetParent offsetLeft F 1.5 BODY 64 IE 7 DIV 16 O 9 HTML 48 Given the result from IE I'd say you have to take the first in visual order. I'm not entirely sure about this though. I haven't really checked how CSS works together with RTL etc. Regarding the SVG stuff, I can't really tell from the SVG specification how they expect that to work. For <svg:foreignobject xlink:href=""> it's quite clear I guess, but when you use HTML inline it's unclear to me what should happen. Does it create a new viewport or do you expect to be able to overlay the SVG content with CSS styled elements etc?
Assignee | ||
Comment 42•17 years ago
|
||
That's all poorly defined right now for the inline HTML case. The behaviour of position:fixed, background-attachment:fixed etc is completely undefined. Establishing a new viewport is probably the most reasonable thing to do. We probably don't want to try to resolve that as part of the offset* spec, but I think it should be listed as an issue. I don't really care how you resolve the RTL issues as long as you do :-). We can handle visual order or content order with approximately equal ease. Maybe the best thing would be to think about what Web authors would most want --- probably visual order.
Reporter | ||
Comment 43•17 years ago
|
||
(In reply to comment #39) > Clarified the specification a bit with regard to some of the comments. For > inline elements that generate multiple boxes you take the first box and > calculate from there. That seems to be what browsers do at the moment. > That is incorrect (or I missed what you are referring to). If an element creates multiple boxes offsetWidth/Height is returned as the bounding box of all those boxes. offsetLeft/Top is returned as the position of the first box.
Assignee | ||
Comment 44•17 years ago
|
||
Wow, so offsetLeft/Top/Width/Height don't actually describe any rectangle? That sucks!
Comment 45•17 years ago
|
||
Erik, this is just about offsetLeft and offsetTop. For the new API you can't directly reuse offsetLeft and offsetTop I think. Mostly because of the lovely first implementation and the web that evolved around that "the <body> element" is special. When offsetParent of a certain element becomes <body> you have to calculate its position against the initial containing block, not the <body> element. When the element itself is <body> offsetParent has to be null and offsetLeft and offsetParent are 0, always. So for a positioned <body> element you can't ever get the position using offsetLeft and offsetTop. When we tried to work around those limitations in Opera by providing a "better" solution we broke the web. (This is part of the specification now so no "new" implementations have to break the web before "getting it right.") And if we get an API to do these things more properly than with offset* I'd like to see that it addresses all cases at least. Roc, noted rotated content inside <svg:foreignObject> as an open issue.
Assignee | ||
Comment 46•17 years ago
|
||
(In reply to comment #45) > When offsetParent of a certain > element becomes <body> you have to calculate its position against the initial > containing block, not the <body> element. When the element itself is <body> > offsetParent has to be null and offsetLeft and offsetParent are 0, always. I see you've just revised your spec to do this :-). So with the current revision of your spec, if you sum up the offsetLeft/offsetTop values all the way up the offsetParent chain, you always get the offset of the starting element's border-box left/top from the origin of the initial containing block --- UNLESS the starting element was the BODY, in which case you get (0,0). [I guess you also get something weird for image maps and their parts, but that doesn't really worry me because they're not rendered elements in the normal sense.] I guess I could implement that quirk for getClientRects/getBoundingClientRect, but should I? Hmm.
Assignee | ||
Comment 47•17 years ago
|
||
According to MSDN the rectangle(s) returned by getBoundingClientRect/getClientRects contain just left/top/right/bottom properties. should we have x/y/width/height as well?
Assignee | ||
Comment 48•17 years ago
|
||
Another interesting question is whether these properties should always be integers or whether we should allow floating point results. CSS does.
Assignee | ||
Comment 49•17 years ago
|
||
Yet another important question is what element types should expose getBoundingClientRect/getClientRects. Right now I just have them on HTML elements (where offset* are also defined), but maybe we should put them on all elements.
Assignee | ||
Comment 50•17 years ago
|
||
Another issue is whether the rectangle returned for a table should include the caption, which happens to be outside the table borders.
Assignee | ||
Comment 51•17 years ago
|
||
Implement getClientRects/getBoundingClientRect. This is the simple implementation without any quirks (I hope!). In particular BODY is not at (0,0) in general. getClientRects returns rectangles in content order, simply because this turned out to be more convenient for me.
Assignee: general → roc
Status: NEW → ASSIGNED
Assignee | ||
Comment 52•17 years ago
|
||
This patch returns floating point coordinates when things are subpixel positioned, up to the resolution of our internal coordinates. For tables, it includes the caption, which is arguably wrong. If someone wants a build with this patch for testing, and you aren't able to make one yourself, jump onto Mozilla IRC and you might be able to find someone to create a test build for you.
Assignee | ||
Comment 53•17 years ago
|
||
What I'd like to do, if we can get the spec and implementation of these methods nailed down soon, is land this on trunk and also on FF2 branch. Then after FF2 has shipped I would disable getBoxObjectFor for Web content on the trunk for FF3.
Comment 54•17 years ago
|
||
(In reply to comment #50) > Another issue is whether the rectangle returned for a table should include > the caption, which happens to be outside the table borders. http://dump.testsuite.org/2006/dom/style/offset/057.htm http://dump.testsuite.org/2006/dom/style/offset/058.htm The expected results where not really what I expected, but apparently O9, IE7 and F1.5 all think they're perfect. So when it includes a caption you calculate from border edge of the caption and not the table (or perhaps the top border edge of the "table box"?). Also, what is the difference between left, top and x, y? And width and height can be easily calculated if you really need them. I'd be nice if this could work for all elements, not just HTML elements, imho. Please leave out the BODY quirk (as you've done now :-) ). As opposed to offset* etc. this can hopefully be a more sane API.
Assignee | ||
Comment 55•17 years ago
|
||
Okay. So the remaining issues are: -- Making it work for all elements. What should we do about SVG elements? -- Foreignobject: I think we've agreed that foreignobject establishes a new reference coordinate system for the content inside it. -- Should we return integers only, or allow floats to be returned? -- Should we make top/bottom/left/right mutable, as IE does, or readonly?
Assignee | ||
Comment 56•17 years ago
|
||
anyone want to suggest an opinion on those questions?
Comment 57•17 years ago
|
||
For SVG you could take the smallest rectangle the figure fits in. For paths and such you could let getClientRects return multiple objects I guess representing the multiple points. I'd say we allow floats to be returned for precision. Given that these interfaces are used in JavaScript it shouldn't do any harm. I'd also propose to make top/bottom/etc. readonly unless we get a very good use case not to do that.
Assignee | ||
Comment 58•17 years ago
|
||
Alright, this patch does that. I'm going for review now. Requesting r from jst as DOM peer for the new DOM API, and sr from tor for the SVG bits
Attachment #226276 -
Attachment is obsolete: true
Attachment #228780 -
Flags: superreview?
Attachment #228780 -
Flags: review?(jst)
Assignee | ||
Updated•17 years ago
|
Attachment #228780 -
Flags: superreview? → superreview?(tor)
Assignee | ||
Comment 59•17 years ago
|
||
tor, I put the new function in nsSVGUtils because I didn't want to export more stuff from layout/svg.
Assignee | ||
Comment 60•17 years ago
|
||
Anne, two things to note: -- The returned rectangles do not include the bounds of any child elements that might happen to overflow. -- For HTML AREA elements, SVG elements that do not render anything themselves, display:none elements, and generally any elements that are not directly rendered, we return an empty list or a (0,0,0,0) rect.
Assignee | ||
Comment 61•17 years ago
|
||
Anne: also: -- for getClientRects we return rectangles even for CSS boxes that have empty border-boxes. The left, top, right and bottom coordinates can still be meaningful. -- for getBoundingClientRect, we completely ignore CSS boxes that have empty border-boxes. If all the element's CSS boxs' border-boxes are empty, then we return a rectangle whose width and height are zero and whose top and left are the top-left of the border-box for the first (content order) CSS box for the element.
Comment 62•17 years ago
|
||
Comment on attachment 228780 [details] [diff] [review] updated patch From an SVG point of view this looks ok, though we be returning the "inflated" pixel rounded covered region rather the fractional coverage. To backport to the branch, GetOuterSVGFrameAndCoveredRegion will need some work to look inside a svg region. You will need to add some ifdefs to handle non-SVG builds. Probably the easiest way of doing that would be to ifdef the body of TryGetSVGBoundingRect. With that, sr=tor for the SVG portion.
Attachment #228780 -
Flags: superreview?(tor) → superreview+
Assignee | ||
Comment 63•17 years ago
|
||
This gives slightly wrong results for SVG without the fix in bug 344206.
Depends on: 344206
Assignee | ||
Comment 64•17 years ago
|
||
Comment on attachment 228780 [details] [diff] [review] updated patch Jonas, if you could review this before jst gets to it, that would be great.
Attachment #228780 -
Flags: review?(bugmail)
Comment on attachment 228780 [details] [diff] [review] updated patch >Index: layout/svg/base/src/nsSVGUtils.h >+ /** >+ * Get the covered region for a frame. Return null if it's not an SVG frame. >+ * @param aRect gets a rectangle in *pixels* >+ * @return the outer SVG frame which aRect is relative to >+ */ >+ static nsIFrame* >+ GetOuterSVGFrameAndCoveredRegion(nsIFrame* aFrame, nsRect* aRect); No description of the aFrame param. Also, please add a comment saying that aRect is an out-param. >Index: content/base/src/nsGenericElement.cpp >+NS_IMPL_ISUPPORTS1(nsDOMNSElementTearoff, nsIDOMNSElement) This is not right. You want to be able to QI back to any interface the element implements even after having QIed to nsIDOMNSElement. See nsGenericHTMLElementTearoff. >+static nsPoint >+GetOffsetFromInitialContainingBlock(nsIFrame* aFrame) >+{ >+ nsPresContext* presContext = aFrame->GetPresContext(); >+ nsIPresShell* shell = presContext->PresShell(); >+ nsIFrame* rootScrollFrame = shell->GetRootScrollFrame(); >+ nsPoint pt(0,0); >+ nsIFrame* child = aFrame; >+ for (nsIFrame* p = aFrame->GetParent(); p && p != rootScrollFrame; >+ p = p->GetParent()) { >+ pt += p->GetPositionOfChildIgnoringScrolling(child); >+ // coordinates of elements inside a foreignobject are relative to the top-left >+ // of the nearest foreignobject >+ if (p->IsFrameOfType(nsIFrame::eSVGForeignObject)) >+ return pt; >+ child = p; >+ } >+ return pt; >+} >+ >+static double >+RoundFloat(double aValue) >+{ >+ return floor(aValue + 0.5); Does this need to be |0.5f|? Feel free to make this function inline. >+TryGetSVGBoundingRect(nsIFrame* aFrame, nsRect* aRect) >+nsDOMNSElementTearoff::GetBoundingClientRect(nsIDOMTextRectangle** aResult) >+nsDOMNSElementTearoff::GetClientRects(nsIDOMTextRectangleList** aResult) I would rather that someone else reviewed these functions as I don't know our layout code well enough to verify that they do the right thing. One thing I did note though was that GetClientRects does not return a live list, was this intended? Most current functions in the DOM, such as .childNodes or .frames return live lists. I don't really have an oppinion on if this should be one way or the other, but I wanted to make sure it was an intended desition. I like the fact that the list is returned in a different manner though (from a function rather than from a property), which IMHO makes it more ok that it is non-live. r=me with a comment on the above question.
Attachment #228780 -
Flags: review?(jst)
Attachment #228780 -
Flags: review?(bugmail)
Attachment #228780 -
Flags: review+
Assignee | ||
Comment 66•17 years ago
|
||
(In reply to comment #65) > No description of the aFrame param. Also, please add a comment saying that > aRect is an out-param. OK > >+NS_IMPL_ISUPPORTS1(nsDOMNSElementTearoff, nsIDOMNSElement) > > This is not right. You want to be able to QI back to any interface the element > implements even after having QIed to nsIDOMNSElement. See > nsGenericHTMLElementTearoff. OK > >+static double > >+RoundFloat(double aValue) > >+{ > >+ return floor(aValue + 0.5); > > Does this need to be |0.5f|? No > Feel free to make this function inline. It's 'static' so the compiler should be able to do the right thing, whatever that is. > >+TryGetSVGBoundingRect(nsIFrame* aFrame, nsRect* aRect) > >+nsDOMNSElementTearoff::GetBoundingClientRect(nsIDOMTextRectangle** aResult) > >+nsDOMNSElementTearoff::GetClientRects(nsIDOMTextRectangleList** aResult) > > I would rather that someone else reviewed these functions as I don't know our > layout code well enough to verify that they do the right thing. OK > One thing I did note though was that GetClientRects does not return a live > list, was this intended? Yes. The individual rects have no identity of their own, unlike in the DOM node lists. It's really just a representation of a shape.
Assignee | ||
Comment 67•17 years ago
|
||
Comment on attachment 228780 [details] [diff] [review] updated patch Can you review just these functions >+TryGetSVGBoundingRect(nsIFrame* aFrame, nsRect* aRect) > >+nsDOMNSElementTearoff::GetBoundingClientRect(nsIDOMTextRectangle** aResult) > >+nsDOMNSElementTearoff::GetClientRects(nsIDOMTextRectangleList** aResult)
Attachment #228780 -
Flags: review?(dbaron)
Comment 68•17 years ago
|
||
(In reply to comment #65) > (From update of attachment 228780 [details] [diff] [review] [edit]) > >+TryGetSVGBoundingRect(nsIFrame* aFrame, nsRect* aRect) > >+nsDOMNSElementTearoff::GetBoundingClientRect(nsIDOMTextRectangle** aResult) > >+nsDOMNSElementTearoff::GetClientRects(nsIDOMTextRectangleList** aResult) > > I would rather that someone else reviewed these functions as I don't know our > layout code well enough to verify that they do the right thing. So apparently I've been asked to do this (a number of months ago, in comment 67). I've started to try to do this a few times. And like I just did, I look at the patch for a few minutes, or part of an hour, and read through the bug comments for similar amounts of time, and as of yet I've been unable to figure out what "the right thing" is, or if anybody knows what it is. But I can't even tell from the maze of comments whether this is intended to be IE-compatible or whether it's a new invention. And either way I don't see a single word of documentation about what these two methods do other than the MSDN URLs. Sorry for the huge delay here -- I tend to stall rather than give feedback on review requests that I don't know how to review -- simply because I think I should be able to figure out how to review them, given just a little more time. I was actually going to rubber-stamp this in the hopes that somebody else knows what's going on, but now I'm having second thoughts about that given that I haven't found any documentation of what these methods do other than what's on MSDN (Anne's spec only seems to cover offset*, not these methods), and the comments seem to indicate that this is *not* an attempt to be IE-compatible, but is rather a new invention, I don't see how this is going to be much use to Web developers without *some* indication of what it's intended to do (at the very least so they can distinguish bug from feature). Could somebody summarize: * what are these methods intended to do? * what quirky IE behavior are we trying to match? * what quirky IE behavior are we choosing not to match? * how carefully has this been tested for IE-compatibility? * how carefully has this been tested for what we think it's supposed to do? It would probably be good if the patch had some comments in nsIDOMNSElement.idl describing the methods, or at least a pointer to a spec for them.
Comment 69•17 years ago
|
||
Comment on attachment 228780 [details] [diff] [review] updated patch I think the behavior described in comment 61 is almost definitely a bug. Ignoring rects that happen to have 0 width or height doesn't make sense in some contexts. But there are other contexts where it will probably cover up some layout bugs we have. Other than that, I think the behavior in this patch seems pretty reasonable. But it still seems sad not to have a spec for it. r=dbaron since it's at least as good as getBoxObjectFor, though.
Attachment #228780 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 70•17 years ago
|
||
Sorry that things weren't clear. > * what are these methods intended to do? The intended behaviour is: -- getClientRects returns the CSS border-boxes for the element, in CSS pixels, with the top left measured relative to the top left of the canvas, assuming everything scrolled to its default position (per comment #34), unless the element is a descendant of an SVG foreignobject, in which case the rectangles are measured relative to the origin of the nearest foreignobject ancestor, in the foreignobject subtree's CSS coordinate system (i.e. ignoring any transforms applied to the foreignobject or its ancestors). -- getBoundingClientRect returns the union of those rectangles. Comments #60 and #61 apply to resolve some ambiguities in the above definition and to clarify some properties that might not be obvious from the definition: -- The returned rectangles do not include the bounds of any child elements that might happen to overflow (ok, this was not ambiguous in the above definition, it's just a note) -- For HTML AREA elements, SVG elements that do not render anything themselves, display:none elements, and generally any elements that are not directly rendered, we return an empty list or a (0,0,0,0) rect (arguably this also follows from the definition) -- for getClientRects we return rectangles even for CSS boxes that have empty border-boxes. The left, top, right and bottom coordinates can still be meaningful. -- for getBoundingClientRect, we completely ignore CSS boxes that have empty border-boxes. If all the element's CSS boxs' border-boxes are empty, then we return a rectangle whose width and height are zero and whose top and left are the top-left of the border-box for the first (content order) CSS box for the element. > * what quirky IE behavior are we trying to match? No particular IE quirks are being specifically accounted for. If I understand what I've been told correctly, there is limited IE compatibility to the extent that "elem.getBoundingClientRect().left - document.body.getBoundingClientRect().left" (and ditto for .top) is equal in both implementations (modulo IE bugs that I haven't been told about). > * what quirky IE behavior are we choosing not to match? Setting document.body.getBoundingClientRect().left (and .top) to 2 or 0 depending on standards vs quirks mode and making everything relative to that. > * how carefully has this been tested for IE-compatibility? It hasn't. It would be useful to test for the "relative to body" compatibility property that I mentioned above. > * how carefully has this been tested for what we think it's supposed to do? I tested individual cases that I thought of, including cases I mentioned here and tested for in the code. > I think the behavior described in comment 61 is almost definitely a bug. OK, we can easily change it and it sounds like we should.
Comment 71•17 years ago
|
||
We also made the attributes of Textrectangle readonly. (Because writing them didn't do anything in Internet Explorer and therefore it didn't make much sense for them not to be readonly.) My plan is to standardize these in the CSSOM: http://dev.w3.org/cvsweb/csswg/cssom/
Assignee | ||
Comment 72•17 years ago
|
||
So Opera's implemented this?
Comment 73•17 years ago
|
||
Oops, confusing use of "we". I was referring to our previous discussion on this when "we" (you and I) decided to change that as well. I do think we'll implement this in due course. Not sure when.
Updated•17 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 74•16 years ago
|
||
Attachment #228780 -
Attachment is obsolete: true
Comment 75•16 years ago
|
||
Thanks, roc. However to get the build buildable, I had to change this: - BREAK_WHITESPACE = 0x01, + BREAK_WHITESPACE_END = 0x01, - PRPackedBool mBreakBeforeNextWord; + PRPackedBool mBreakBeforeNonWhitespace; in nsLineBreaker.h.
Assignee | ||
Comment 76•16 years ago
|
||
Sorry, that crept in by mistake.
Comment 77•16 years ago
|
||
And it wont actually build with SVG disabled... How much does it depend on SVG?
Assignee | ||
Comment 78•16 years ago
|
||
Not much ... something like this should work +static PRBool +TryGetSVGBoundingRect(nsIFrame* aFrame, nsRect* aRect) +{ +#ifdef MOZ_ENABLE_SVG + nsRect r; + nsIFrame* outer = nsSVGUtils::GetOuterSVGFrameAndCoveredRegion(aFrame, &r); + if (!outer) + return PR_FALSE; + + // r is in pixels relative to 'outer', get it into twips + // relative to ICB origin + r.ScaleRoundOut(1.0/aFrame->PresContext()->AppUnitsPerCSSPixel()); + *aRect = r + GetOffsetFromInitialContainingBlock(outer); + return PR_TRUE; +#else + return PR_FALSE; +#endif +} along with #ifdefs protecting #include "nsSVGUtils.h" of course
Comment 79•16 years ago
|
||
I'm trying to make Mochitests for the patch. One thing I noticed: http://martijn.martijn.googlepages.com/display_none.htm I get this error with the patch: Error: uncaught exception: [Exception... "Illegal value" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: In IE7 it alerts undefined. I also get this error with the patch with this example: http://martijn.martijn.googlepages.com/display_none_body.htm It returns '0-0-0-0' in IE7. document.documentElement.getBoundingClientRect().left - document.body.getBoundingClientRect().left is -8 in Mozilla with the patch, in IE7 it is 0. So that's a case where it isn't compatible. Like you mentioned in comment 66, you made the rect lists non-live. That's not what IE is doing. Maybe it is wiser to be compatible with IE in this regard, to avoid confusion with web developers? I'm still working on more testcases.
Assignee | ||
Comment 80•16 years ago
|
||
Thanks Martijn, this is exactly what I needed :-) I'll look into those issues. One question: IE makes the rect list live? Do you mean the list is live, but the rects aren't? Making the list live is probably not hard.
Assignee | ||
Comment 81•16 years ago
|
||
One question about live lists, though: if we make it a live list, when should the objects in the list change? E.g. what happens if the author pulls out a rectangle, keeps a reference to it, and changes the layout, then looks at the rectangles in the list again, possibly comparing the identity of those objects with the old rectangle(s)? What sort of layout changes cause the list to be filled with new objects? If we don't use a live list, we don't have to specify that.
Assignee | ||
Comment 82•16 years ago
|
||
(In reply to comment #79) > I'm trying to make Mochitests for the patch. > One thing I noticed: http://martijn.martijn.googlepages.com/display_none.htm > I get this error with the patch: > Error: uncaught exception: [Exception... "Illegal value" nsresult: "0x80070057 > (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: > In IE7 it alerts undefined. Fixed in an upcoming patch. > I also get this error with the patch with this example: > http://martijn.martijn.googlepages.com/display_none_body.htm > It returns '0-0-0-0' in IE7. I think this should return undefined like other elements that have no CSS boxes. This is probably an IE bug related to its broken handling of BODY. > document.documentElement.getBoundingClientRect().left - > document.body.getBoundingClientRect().left is -8 in Mozilla with the patch, in > IE7 it is 0. So that's a case where it isn't compatible. OK. We need to restrict the compatibility guarantee to elements that are descendants of the body. Thanks!
Assignee | ||
Comment 83•16 years ago
|
||
Attachment #265652 -
Attachment is obsolete: true
Comment 84•16 years ago
|
||
" Like you mentioned in comment 66, you made the rect lists non-live. That's not what IE is doing. Maybe it is wiser to be compatible with IE in this regard, to avoid confusion with web developers? " Ok, this part of my comment 79 is wrong. See these rough examples: http://martijn.martijn.googlepages.com/boundingrect3.html http://martijn.martijn.googlepages.com/display_none_d.htm The seem to indicate that IE doesn't do anything live, not the getClientRects() list, nor the rect itself.
Assignee | ||
Comment 85•16 years ago
|
||
Martijn, are you planning to do anything more here, or should I just land it?
Assignee | ||
Comment 86•16 years ago
|
||
OK, I landed this. Any remaining issues can be addressed in followup bugs. Thanks everyone!
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite?
Comment 87•16 years ago
|
||
I cleaned up the specification a bit more: http://dev.w3.org/cvsweb/~checkout~/csswg/cssom/Overview.html?content-type=text/html;%20charset=utf-8#elementlayout Let me know if you guys have any input!
Assignee | ||
Comment 88•16 years ago
|
||
A couple of suggestions: -- should specify the handling of content inside a foreignobject (in a way that could be extended to elements that apply other complex transformations); see comment #70. What you have right now isn't very useful in those situations. -- should specify how empty boxes are treated, hopefully like I did in comment #70 :-)
Comment 89•16 years ago
|
||
Doesn't foreignObject just become the initial containing block? (Someone really needs to define foreignObject...) Could you give an example that would create empty border boxes? Just an element with height and width of zero and no border or padding?
Assignee | ||
Comment 90•16 years ago
|
||
(In reply to comment #89) > Doesn't foreignObject just become the initial containing block? Maybe that's the right way to define it. I don't know. > Could you give an example that would create > empty border boxes? Just an element with height and width of zero and no border > or padding? Sure. Or a <span style="font-size:0">, I guess. That could create multiple empty border-boxes.
Comment 91•16 years ago
|
||
Clarified the empty boxes situation (to match comment 70) and added a note for svg:foreignObject.
Assignee | ||
Comment 92•16 years ago
|
||
Thanks. Martijn, you were going to turn your tests into Mochitests, right?
Comment 93•16 years ago
|
||
Yeah. You shouldn't worry about those. Although I might ask you review for them. I really should finish it by tomorrow (or just ask review on what I've done so far).
Comment 94•16 years ago
|
||
Sorry for being dense, is this going in to FF3?
Comment 95•16 years ago
|
||
(In reply to comment #94) > Sorry for being dense, is this going in to FF3? > Yes
![]() |
||
Comment 96•16 years ago
|
||
Could this possibly have caused bug 392671 ?
Comment 97•16 years ago
|
||
IE provides bounding properties and methods also for their TextRange object. I think it would make sense and be useful to extend the Range object similarily. http://msdn2.microsoft.com/en-us/library/ms535872.aspx
Comment 98•16 years ago
|
||
Ivan, please file a new bug for that and mention the bug number here.
Comment 99•16 years ago
|
||
Some documentation is at: http://developer.mozilla.org/en/docs/DOM:element.getClientRects http://developer.mozilla.org/en/docs/DOM:element.getBoundingClientRect It isn't very clear though and could use some examples.
Target Milestone: --- → mozilla1.9alpha5
Comment 100•16 years ago
|
||
It looks like this caused bug 396694
Comment 101•16 years ago
|
||
The docs for this have been twiddled and are pretty acceptable now. Marking as dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
Updated•15 years ago
|
Keywords: helpwanted
Updated•10 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•