Last Comment Bug 340571 - getBoxObjectFor leaking-onto-the-Web disaster
: getBoxObjectFor leaking-onto-the-Web disaster
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla1.9.2a1
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Hixie (not reading bugmail)
Mentors:
: 159457 (view as bug list)
Depends on: 174397 401549 409220 486200 539265
Blocks: 340510
  Show dependency treegraph
 
Reported: 2006-06-06 13:55 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2010-12-22 03:39 PST (History)
34 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (28.95 KB, patch)
2009-03-23 17:30 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-06 13:55:10 PDT
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) (Exited; email my personal email if necessary) 2006-06-06 13:55:57 PDT
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) 2006-06-06 14:16:04 PDT
1a seems ok, anyone know how the IE API works?
Comment 5 David Hyatt 2006-06-06 14:38:33 PDT
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) (Exited; email my personal email if necessary) 2006-06-06 15:13:31 PDT
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) (Exited; email my personal email if necessary) 2006-06-06 15:15:25 PDT
And I presume this API should be defined in terms of CSS borders (i.e., should ignore outline).
Comment 8 Anne (:annevk) 2006-06-06 16:07:34 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-06-07 06:38:30 PDT
What are the implications of getBoundingClientRect? That sounds like a "good" solution to me.
Comment 10 Sylvain Pasche 2006-06-07 10:32:27 PDT
Some informative comments about getBoundingClientRect can be found in bug 174397
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-07 13:02:04 PDT
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 2006-06-07 15:52:27 PDT
MochiKit tracking bug:

http://trac.mochikit.com/ticket/125
Comment 13 John Resig 2008-03-18 10:59:42 PDT
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 2008-03-18 16:38:02 PDT
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) (Exited; email my personal email if necessary) 2008-03-21 18:41:34 PDT
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 [:bz] 2009-03-23 13:05:44 PDT
So we were going to remove this in 1.9.1, right?  :(

nominating for blocking 1.9.2.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-23 17:30:55 PDT
Created attachment 368994 [details] [diff] [review]
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 [:bz] 2009-03-24 09:46:54 PDT
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) (Exited; email my personal email if necessary) 2009-03-30 00:38:13 PDT
http://hg.mozilla.org/mozilla-central/rev/ef71595d291a
Comment 20 neil@parkwaycc.co.uk 2009-03-30 01:12:06 PDT
(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) (Exited; email my personal email if necessary) 2009-03-30 02:30:20 PDT
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) 2009-03-30 07:31:15 PDT
Could this have broken smoothwheel extension?
Comment 23 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-03-30 07:34:57 PDT
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) 2009-03-30 08:01:00 PDT
Thanks, I have informed the author about the issue.
Comment 25 Giorgio Maone [:mao] 2009-03-31 14:04:08 PDT
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 [:mao] 2009-03-31 14:44:56 PDT
Any chance to have this patch backed out until bug 486200 gets fixed?
Comment 27 karl 2009-03-31 15:23:34 PDT
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) (Exited; email my personal email if necessary) 2009-03-31 17:10:24 PDT
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) (Exited; email my personal email if necessary) 2009-03-31 17:15:54 PDT
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 [:mao] 2009-03-31 18:15:12 PDT
(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 2009-04-16 09:20:20 PDT
This also broke snaplinks.
Comment 32 Christopher Blizzard (:blizzard) 2009-08-10 15:39:18 PDT
Turns out that mootools uses this:

https://mootools.lighthouseapp.com/projects/2706-mootools/tickets/155
Comment 33 :Ms2ger 2010-12-22 03:39:23 PST
*** Bug 159457 has been marked as a duplicate of this bug. ***

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