Closed Bug 174397 Opened 21 years ago Closed 16 years ago

Support for getClientRects and getBoundingClientRect

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha5

People

(Reporter: erik, Assigned: roc)

References

Details

(Keywords: dev-doc-complete, testcase)

Attachments

(4 files, 4 obsolete files)

Move the mouse around and notive statusbar and the dotted borders
8.71 KB, text/html
Details
Another test case that shows some interesting relations
2.49 KB, text/html
Details
getBoundingClientRect/offsetLeft/offsetTop tests
4.89 KB, text/html
Details
update
35.91 KB, patch
Details | Diff | Splinter Review 
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
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
The rect used is the border-box relative to the client viewport

I'll be willing to create the needed test cases.
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.
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).
-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.
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.)
QA Contact: lchiang → ian
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.
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..
Oops, I meant getClientRects() get the rects for wrapped text, in last post.
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.
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
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
"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.
> 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
(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 ;-)
Any chance of this bug being fixed for Gecko 1.8?
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...
bz: that sounds reasonable.
*** Bug 323078 has been marked as a duplicate of this bug. ***
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.
I can fix "poorly specified"; can you expand on "nasty quirks"?
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 :-)
If that's it, then that's not enough to invent a whole new API, IMHO.
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.
Note, apparently whether there is a 2px offset or not depends on quirks vs standards mode.
That's even better, it means we can implement it without feeling bad. :-)
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.
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?
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).
I would imagine that anything we implement would be very complicated, even without quirks.
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.
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).
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 :-).
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).
(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.
Attachment #224960 - Attachment is obsolete: true
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.)
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.
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?

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?
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.
(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.
Wow, so offsetLeft/Top/Width/Height don't actually describe any rectangle? That sucks!
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.
(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.
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?
Another interesting question is whether these properties should always be integers or whether we should allow floating point results. CSS does.
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.
Another issue is whether the rectangle returned for a table should include the caption, which happens to be outside the table borders.
Attached patch first cut patch (obsolete) — — Splinter Review 
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
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.
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.
(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.
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?
anyone want to suggest an opinion on those questions?
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.
Attached patch updated patch (obsolete) — — Splinter Review 
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)
Attachment #228780 - Flags: superreview? → superreview?(tor)
tor, I put the new function in nsSVGUtils because I didn't want to export more stuff from layout/svg.
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.
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 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+
This gives slightly wrong results for SVG without the fix in bug 344206.
Depends on: 344206
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+
(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.
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)
(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 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+
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.
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/
So Opera's implemented this?
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.
Keywords: dev-doc-needed
Blocks: 364612
Attached patch updated to trunk (obsolete) — — Splinter Review 

        Attachment #228780 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
Sorry, that crept in by mistake.

    
    

    
  
And it wont actually build with SVG disabled... How much does it depend on SVG?

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
(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!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        update
        — 
          — Splinter Review
      

    


  

        Attachment #265652 -
        Attachment is obsolete: true

    
    

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

    
    

    
  
Martijn, are you planning to do anything more here, or should I just land it?

    
    

    
  
OK, I landed this. Any remaining issues can be addressed in followup bugs. Thanks everyone!
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
  
Flags: in-testsuite?

    
    

    
  
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!

    
    

    
  
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 :-)

    
    

    
  
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?

    
    

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

    
    

    
  
Clarified the empty boxes situation (to match comment 70) and added a note for svg:foreignObject.

    
    

    
  
Thanks. Martijn, you were going to turn your tests into Mochitests, right?

    
    

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

    
    

    
  
Sorry for being dense, is this going in to FF3?

    
    

    
  
(In reply to comment #94)
> Sorry for being dense, is this going in to FF3?
> 

Yes


    
    

    
  
Could this possibly have caused bug 392671 ?
Depends on: 392671

    
    

    
  
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

    
    

    
  
Ivan, please file a new bug for that and mention the bug number here.

    
    

    
  
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
Depends on: 396694

    
    

    
  
It looks like this caused bug 396694

    
    

    
  
The docs for this have been twiddled and are pretty acceptable now.  Marking as dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
Keywords: helpwanted

    
  
Depends on: 487597
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.