Closed Bug 486200 Opened 11 years ago Closed 10 years ago

Need API to compute screen coordinates of DOM elements

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mao, Assigned: roc)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

Bug 340571 (removing getBoxObjectFor from HTML documents) broke NoScript, making all the pages containing cross-site IFrames (e.g. Hotmail) completely unusable.

The reason is ClearClick, which needs to compute screen coordinates for DOM elements in order to take a pair pixel-precise Canvas "screenshots"" of the same display area, but from two different points of view: the top-level window and the frame window. To do so, the only reliable references even across multiple nested frames with different scrolling position are screen coordinates offsets.

I don't know if any other getBoxObjectFor() client has been left in the cold for the same reason, but removing that API under the assumption getBoundingClientRect() might be a replacement clearly overlooked the screen coordinates access provided by the box object.
My plan here is to add window.mozInnerScreenX/Y properties that return the screen coordinates of the top-left of the viewport. It should be trivial, I'll do it ASAP.
How are we exactly supposed to compute the element screenX / screenY from client rect + window coords, *taking in account zoom*?

In my (quite extensive) experience browser.markupDocumentViewer.fullZoom was unreliable, leading to miscalculations. I may have choosen the wrong API there in first place (please advise if I did), but to obtain pixel-perfect results I had to compute the "real" zoom factor by myself the following hack (which requires DOMElement.screenX/screenY):

var root = d.documentElement;
var o = d.createElementNS(HTML_NS, "div");
with(o.style) {
  top = "400000px";
  position = "absolute";
  display = "block";
}
root.appendChild(o);
var oBox = d.getBoxObjectFor(o);
var rootBox = d.getBoxObjectFor(root);
var zoom = (oBox.screenY - rootBox.screenY) / o.offsetTop;
root.removeChild(o);

Of course if you expose only window's coord rather than a wrapper on the old API, this hack won't work anymore...
Flags: blocking1.9.2?
I'm not sure what boxObject.screenX/Y does, but mozInnerScreenX/Y would return a value in CSS pixels ... it should just work.
In Gecko 1.9.0, boxObject.screenX/screenY of HTML elements return the actual position on the screen. When markupDocumentViewer.fullZoom is modified, boxObject.screenX/screenY are also changed. For example, for the element which has "margin-left:100px" and markupDocumentViewer.fullZoom is 0.5, nsIDOMClientRect.left returns "100" but nsIBoxObject.screenX returns "50". Even if the window gets new properties like "mozInnerScreenX/Y", it is hard to calculate the actual position of an HTML element from its nsIDOMClientRect's properties and the new "mozInnerScreenX".
Giorgio doesn't want *actual* screen coordinates, I think. I think he just wants to calculate the offset between one window and another, where both windows are in the same tab with the same zoom level.

I can probably provide another value, say window.mozScreenPixelsPerCSSPixel, which will let you convert from CSS pixels to actual physical screen pixels.
Attached patch fix (obsolete) — Splinter Review
Fairly straightforward. We want these properties to be floats since if the zoom is 1.5 (for example) then it's normal for the screen position to include some fraction of a CSS pixel.
Attachment #370567 - Flags: superreview?(jst)
Attachment #370567 - Flags: review?(dbaron)
We should probably quickstub these, no? js/src/xpconnect/src/dom_quickstubs.qsconf is where that would go.
No other nsIDOMWindowInternal methods are quickstubbed, so I suspect that would be bad.
Any hope for this one to land any time soon?
Yeah, as soon as it gets reviewed. I don't expect that to be long.
Comment on attachment 370567 [details] [diff] [review]
fix

Could you document in the IDL that the values are in CSS pixels?

r=dbaron.
Attachment #370567 - Flags: review?(dbaron) → review+
Comment on attachment 370567 [details] [diff] [review]
fix

I'm not aware of any reason why quickstubbing these would be bad, but we can worry about that when we quickstub other stuff that matters in nsIDOMWindowInternal.

sr=jst
Attachment #370567 - Flags: superreview?(jst) → superreview+
Thanks!
Whiteboard: [needs landing]
Keywords: checkin-needed
Attached patch 486200-2.diffSplinter Review
Roc asked me to update this and make it chrome only, so here it is.
Assignee: roc → db48x
Attachment #370567 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #383927 - Flags: superreview?
Attachment #383927 - Flags: review?
Attachment #383927 - Flags: superreview?(jst)
Attachment #383927 - Flags: superreview?
Attachment #383927 - Flags: review?(roc)
Attachment #383927 - Flags: review?
Duplicate of this bug: 495192
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 383927 [details] [diff] [review]
486200-2.diff

Let's land this!
Attachment #383927 - Flags: superreview?(jst)
Attachment #383927 - Flags: superreview+
Attachment #383927 - Flags: review?(roc)
Attachment #383927 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
sounds good to me… checked in.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
I backed this out, as one of the tests didn't pass on the test machines. See http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1245396606.1245400251.13460.gz

I can't reproduce this on my machines, but if I make the margin of the iframe 50.5px then it comes close.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Users start noticing that this is not fixed yet... 
http://forums.informaction.com/viewtopic.php?f=7&t=1913
Attached patch updated testSplinter Review
This test avoids subpixel vertical offsets for the IFRAME and should pass everywhere.
Assignee: db48x → roc
Whiteboard: [needs landing]
Not holding 1.9.2 for this, but this is ready to land it seems. Roc, need help landing this, or you on it?
Flags: blocking1.9.2? → blocking1.9.2-
http://hg.mozilla.org/mozilla-central/rev/42079bb031e1
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Keywords: dev-doc-needed
Flags: in-testsuite+
Since window.mozScreenPixelsPerCSSPixel is chrome only, did you want to expose it on the content window object?
It kinda belongs with the other properties. Is that bad?
It broke several browser based javascript tests which evaluate properties of the window object. I can wrap the property accesses in a try catch block to hide the problem, but it effect other code "out there".
Why did this need to be chrome only? It's kinda uncool to have properties on all webpages and not let them see the value of it. If this needs to be chrome only, we need to hide this from content, or move it out of the global scope (i.e. into window utils or what not).

Alternatively we could just return early w/o throwing here, and always give the value 1 back, or whatever.
(In reply to comment #28)
> I can wrap the property accesses in a try catch block to
> hide the problem, but it effect other code "out there".

I just got bitten by this too. Basically, that's affecting people doing some introspection on the global object, for instance to retrieve all objects matching a given pattern:

var tests = [];
for (var prop in window) {
  if (typeof(window[prop]) != "object")
    continue;
  if (prop.search(/Test(\d*)$/) != -1) {
    tests.push(prop);
    continue
  }
}
Alright, I'll move it to nsIDOMWindowUtils. Filed bug 507755 on that.

It needs to be chrome-only because I don't want Web authors to have easy access to information about screen pixels. They'll try to defeat our zooming or size things to screen pixels, which we don't want.
We probably should have a mochitest for stuff like comment #30. I'll write one.
Added some documentation to https://developer.mozilla.org/en/Firefox_3.6_for_developers, it isn't enough, but at least it's something :)
Depends on: 507755
Depends on: 831472
There is an open issue in Chrome for this as well: https://code.google.com/p/chromium/issues/detail?id=151983

It seems to be stalled waiting for specification. Any chance you can jump in there, :roc?
Flags: needinfo?(roc)
I don't think writing the spec is the problem.

https://github.com/slightlyoff/PositionObserver/blob/master/explainer.md might be a better API for most of the use-cases for screen coordinates.
Flags: needinfo?(roc)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.