Closed Bug 340571 Opened 14 years ago Closed 11 years ago

getBoxObjectFor leaking-onto-the-Web disaster


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

Not set





(Reporter: roc, Assigned: roc)



(Keywords: dev-doc-complete)


(1 file)

Web sites have discovered getBoxObjectFor lurking on nsIDOMNSDocument and have started using it to get element geometry. This is bad:
-- it's not even a quasi-standard
-- it's not a very convenient API to use or implement because it introduces an extra object
-- in Mozilla, it's only available when XUL is enabled, so some embedded Geckos don't have it --- actually, it's worse, they have the function, but it always fails
-- when it is available, it contains a bunch of XUL-specific methods that don't make sense for HTML but people might try to use anyway

The most notable site using this is Google Calendar, but there are others. At some point someone seems to have recommended its use :-(.

Now people are asking Webkit to implement it.

The cat's out of the bag so none of our options are good:
#1: Move it from Document to XULDocument, ram that fix onto every supported branch, and break the sites that are using it. Only remotely plausible if the functionality can be obtained some supported way, and I'm not sure if that's so. If this is the best option, it needs to be done now.
#1a: Move it to XULDocument and define another API, perhaps IE's getBoundingClientRect, to satisfy users.
#2: Define a minimal boxobject interface that satisfies (most of) the current consumers on the Web. Change our code so that's all you get in non-XUL documents. Make sure that's enabled even when XUL isn't configured.

Thoughts? is the Webkit feature request, which mentions another site using this.
1a seems ok, anyone know how the IE API works?
I'd recommend you guys remove it anyway and implement WinIE's method instead.  The AJAX toolkits will adjust to accommodate your change.  You really really don't want this used on the Web.
What about this awful quirk?

> In Microsoft Internet Explorer 5, the window's upper-left is at 2,2 (pixels)
> with respect to the true client.

Can we make a pact with Webkit to not emulate that?
And I presume this API should be defined in terms of CSS borders (i.e., should ignore outline).
The outline property has no affect on the actual position of elements (in theory). How much are offsetLeft, offsetTop and offsetParent related to this? See for some work on that.
What are the implications of getBoundingClientRect? That sounds like a "good" solution to me.
Some informative comments about getBoundingClientRect can be found in bug 174397
It sounds good to me too but we need a slightly better spec. For example, I assume the returned coordinates are as if everything scrollable was scrolled to its default position, but that's not stated.

There's also the problem that if we implement something with a nice spec, it likely won't match some IE bugs, so people doing "if (getBoundingClientRect) ..." will get something that doesn't behave quite as they expect. If that is a problem, we should implement something with a different name, not implement getBoundingClientRect and codify a bunch of IE bugs.

I'm not sure why IE returns objects named "TextRectangle", or whether that matters. I guess it does and there's no real harm in following that.

I'll put together an implementation of getBoundingClientRect and getClientRects, in bug 174397.
Depends on: 174397
MochiKit tracking bug:
Depends on: 401549
Now that getBoundingClientRect has landed (bug 174397) can this be pushed forward? (Since a proper alternative to this method now exists.)
But 409220 is now fixed (implemented in bug 409111) which adds a warning when using getBoxObjectFor(). After some baking time in Firefox 3, maybe this function could be hidden from content in the next milestone.
Depends on: 409220
We should remove this in the next major release. By then we'll have dropped support for all Firefox versions that don't have getBoundingClientRect. It will probably cause us some compatibility pain but we'll just have to grin and bear it.
So we were going to remove this in 1.9.1, right?  :(

nominating for blocking 1.9.2.
Flags: blocking1.9.2?
Attached patch fixSplinter Review
Moves getBoxObjectFor from nsIDOMNSDocument to nsIDOMXULDocument so only XUL documents expose it to script. The actual implementation lives on in nsDocument and is exposed to C++ via nsIDocument, so that existing C++ code that needs GetBoxObjectFor and should keep working in non-XUL documents can keep using it.

I'm also removing nsIDOMNSXBLFormControl, which means <select> elements will no longer expose getBoxObject.

I pushed this to the try servers to verify that things don't break. I also audited all our getBoxObjectFor users in the tree. There's a couple left in scripts that I haven't touched, but they won't execute since they check for getBoundingClientRect first.
Attachment #368994 - Flags: superreview?(bzbarsky)
Attachment #368994 - Flags: review?(bzbarsky)
Comment on attachment 368994 [details] [diff] [review]

Please rev nsIDocument's IID.

You probably want to quickstub the nsIDOMXULDocument version of getBoxObjectFor.

Add a test that makes sure that in an HTML document getBoxObjectFor throws?

With those changes, looks good.
Attachment #368994 - Flags: superreview?(bzbarsky)
Attachment #368994 - Flags: superreview+
Attachment #368994 - Flags: review?(bzbarsky)
Attachment #368994 - Flags: review+
Closed: 11 years ago
Flags: blocking1.9.2?
Keywords: dev-doc-needed
Resolution: --- → FIXED
Flags: in-testsuite+
(In reply to comment #17)
> Moves getBoxObjectFor from nsIDOMNSDocument to nsIDOMXULDocument so only XUL
> documents expose it to script.
Why do XUL documents need getBoxObjectFor? Users should go via the .boxObject property on XUL elements.
Perhaps they don't, but I wasn't sure how much that would break, and getting rid of it on XUL documents doesn't seem to matter much one way or another.
Could this have broken smoothwheel extension?
Yeah, seems likely, I found this in the extension code (in smoothwheel.js):
var docBox = targetDoc.getBoxObjectFor(insertionNode);
They should use getBoundingClientRect nowadays.
Thanks, I have informed the author about the issue.
Dropping getBoxObjectFor() on content documents completely broke NoScript's ClearClick.
I need screenX and screenY for content DOM elements, and I'm not in inside a mouse event handler.
How can I do? Couldn't this API be made still available to chrome code, or at least screen coordinates be exposed in some other way?
Depends on: 486200
Any chance to have this patch backed out until bug 486200 gets fixed?
We at Interclue also need the screen co-ordinates of a given element. If you're determined to remove getBoxObjectFor() (and I think you should) and the replacement functionality (getBoundingClientRect) is not allowed to include screenX and screenY, then can you give us something else that includes the elements position in relation to the screen.

If not is there any other reliable way of calculating this for a given DOM element. I, for one, have yet to find a reliable way.
There is a horrible hack... insert a XUL element into the target document, call .boxObject.screenX/Y, then use getBoundingClientRect to compute the distance between the XUL element and other elements in the document. But I'll fix 486200 so people won't have to consider that.

I really don't want to back this out. We knew this would break things, the point of checking it in this early in 1.9.2 is to flush out those things before most users are affected.
BTW calling getBoxObjectFor on non-XUL elements has been sending a warning to the error console "Use of getBoxObjectFor() is deprecated" ever since Firefox 3.
(In reply to comment #29)
> BTW calling getBoxObjectFor on non-XUL elements has been sending a warning to
> the error console "Use of getBoxObjectFor() is deprecated" ever since Firefox
> 3.

Deprecating a proprietary API in favor of a semi-equivalent HTML 5 sanctioned one is fine in content, but the point of forbidding it for chrome callers is debatable: in facts you're keeping it for C++ callers through nsIDocument.
Personally I assumed the warning was meant for content-land consumers, not for extension developers and embedders.
This also broke snaplinks.
Target Milestone: --- → mozilla1.9.2a1
Depends on: 539265
Duplicate of this bug: 159457
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.