340571 - getBoxObjectFor leaking-onto-the-Web disaster

Status: RESOLVED FIXED

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

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://bugzilla.opendarwin.org/show_bug.cgi?id=8154 is the Webkit feature request, which mentions another site using this.

Comment 2

[Hixie (not reading bugmail)]

1a seems ok, anyone know how the IE API works?

Comment 3

[Hixie (not reading bugmail)]

http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/getboundingclientrect.asp

Comment 4

[Robert Sayre]

Found this in

Dojo: http://dojotoolkit.org/pipermail/dojo-checkins/2006-April/005479.html
Yahoo JS: http://developer.yahoo.com/yui/docs/dom/overview-summary-Dom.js.html
Plone: http://dev.plone.org/collective/changeset/22283
MochiKit: http://trac.turbogears.org/turbogears/browser/trunk/turbogears/static/js/MochiKit.js?rev=1269

Comment 5

[David Hyatt]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And I presume this API should be defined in terms of CSS borders (i.e., should ignore outline).

Comment 8

[Anne (:annevk)]

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 http://dump.testsuite.org/2006/dom/style/offset/spec for some work on that.

Comment 9

[Benjamin Smedberg]

What are the implications of getBoundingClientRect? That sounds like a "good" solution to me.

Comment 10

[Sylvain Pasche]

Some informative comments about getBoundingClientRect can be found in bug 174397

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Robert Sayre]

MochiKit tracking bug:

http://trac.mochikit.com/ticket/125

Comment 13

[John Resig]

Now that getBoundingClientRect has landed (bug 174397) can this be pushed forward? (Since a proper alternative to this method now exists.)

Comment 14

[Sylvain Pasche]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So we were going to remove this in 1.9.1, right?  :(

nominating for blocking 1.9.2.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 368994 [details] [diff] [review]
fix

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/ef71595d291a

Comment 20

[[e-mail]]

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

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 22

[Ria Klaassen (not reading all bugmail)]

Could this have broken smoothwheel extension?

Comment 23

[Martijn Wargers (dead)]

Yeah, seems likely, I found this in the extension code (in smoothwheel.js):
var docBox = targetDoc.getBoxObjectFor(insertionNode);
They should use getBoundingClientRect nowadays.

Comment 24

[Ria Klaassen (not reading all bugmail)]

Thanks, I have informed the author about the issue.

Comment 25

[Giorgio Maone [:ma1]]

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?

Comment 26

[Giorgio Maone [:ma1]]

Any chance to have this patch backed out until bug 486200 gets fixed?

Comment 27

[karl]

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.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 30

[Giorgio Maone [:ma1]]

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

Comment 31

[Paul-Kenji Cahier]

This also broke snaplinks.

Comment 32

[Christopher Blizzard (:blizzard)]

Turns out that mootools uses this:

https://mootools.lighthouseapp.com/projects/2706-mootools/tickets/155