486200 - Need API to compute screen coordinates of DOM elements

Status: RESOLVED FIXED

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

Description

[Giorgio Maone [:ma1]]

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.

Comment 1

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

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.

Comment 2

[Giorgio Maone [:ma1]]

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

Comment 3

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

I'm not sure what boxObject.screenX/Y does, but mozInnerScreenX/Y would return a value in CSS pixels ... it should just work.

Comment 4

[YUKI "Piro" Hiroshi]

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

Comment 5

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

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.

Comment 6

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

Attachment: fix

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.

Comment 7

[Johnny Stenback (:jst)]

We should probably quickstub these, no? js/src/xpconnect/src/dom_quickstubs.qsconf is where that would go.

Comment 8

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

No other nsIDOMWindowInternal methods are quickstubbed, so I suspect that would be bad.

Comment 9

[Giorgio Maone [:ma1]]

Any hope for this one to land any time soon?

Comment 10

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

Yeah, as soon as it gets reviewed. I don't expect that to be long.

Comment 11

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

dbaron, jst, reviews?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

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

r=dbaron.

Comment 13

[Johnny Stenback (:jst)]

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

Comment 14

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

Thanks!

Comment 15

[Daniel Brooks [:db48x]]

Attachment: 486200-2.diff

Roc asked me to update this and make it chrome only, so here it is.

Comment 17

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

Comment on attachment 383927 [details] [diff] [review]
486200-2.diff

Let's land this!

Comment 18

[Daniel Brooks [:db48x]]

sounds good to me… checked in.

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3ed492a26551

Comment 20

[Daniel Brooks [:db48x]]

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.

Comment 21

[Giorgio Maone [:ma1]]

Users start noticing that this is not fixed yet... 
http://forums.informaction.com/viewtopic.php?f=7&t=1913

Comment 22

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

Attachment: updated test

This test avoids subpixel vertical offsets for the IFRAME and should pass everywhere.

Comment 23

[Johnny Stenback (:jst)]

Not holding 1.9.2 for this, but this is ready to land it seems. Roc, need help landing this, or you on it?

Comment 24

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

I'm on it

Comment 25

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

http://hg.mozilla.org/mozilla-central/rev/42079bb031e1

Comment 26

[Bob Clary [:bc] (inactive)]

Since window.mozScreenPixelsPerCSSPixel is chrome only, did you want to expose it on the content window object?

Comment 27

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

It kinda belongs with the other properties. Is that bad?

Comment 28

[Bob Clary [:bc] (inactive)]

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

Comment 29

[Johnny Stenback (:jst)]

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.

Comment 30

[Sylvain Pasche]

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

Comment 31

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

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.

Comment 32

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

We probably should have a mochitest for stuff like comment #30. I'll write one.

Comment 33

[Nochum Sossonko [:Natch]]

Added some documentation to https://developer.mozilla.org/en/Firefox_3.6_for_developers, it isn't enough, but at least it's something :)

Comment 34

[Eric Shepherd [:sheppy]]

See:

https://developer.mozilla.org/en/DOM/window.mozInnerScreenX
https://developer.mozilla.org/en/DOM/window.mozInnerScreenY
https://developer.mozilla.org/en/DOM/window.mozScreenPixelsPerCSSPixel

Appropriate links have also been added to:

https://developer.mozilla.org/en/DOM/window#Properties

Comment 35

[Alan Kligman]

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?

Comment 36

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

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.