674871 - [highlighter] there is something wrong with iframes

Status: VERIFIED FIXED

(DevTools :: General, defect, P2)

Description

[Paul Rouget [:paul]]

Go to http://paulrouget.com/e/emulators/

Start the Inspector, and move your mouse to the Flash video.
The selection doesn't cover the whole video (sounds like the size of the borders is not taken into account).

Also,
scroll down to make the Flash video half hidden. Move your mouse to the Flash Video. The selection selects a 0px per 0px region on the top left corner of the page.

Comment 1

[Paul Rouget [:paul]]

After some experiments, sounds like it's not related to plugins, but to iframes. IUI_elementFromPoint does take borders and padding into account.

Comment 2

[Paul Rouget [:paul]]

Attachment: patch v1

The problem is that we don't take into account the potential gap created by the borders and the padding of the iframe.

In this fix, I introduce a function that computes the gap between an iframe and its content window.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 550042 [details] [diff] [review]
patch v1

Please mark attachments as patches. :)

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 550042 [details] [diff] [review]
patch v1

Review of attachment 550042 [details] [diff] [review]:
-----------------------------------------------------------------

The problem is correctly identified, as it looks. The fix is not quite there yet.

Giving the patch r- because it doesn't seem to work properly.

1. On the page you mention I see the highlghter only covering half of the iframe, which is wrong. If I scroll the page, the highlighter height changes for no apparent reason.

2. I have an infamous "stress test" page that I always load when I play with devtools. This page is a mess (on purpose) that holds lots of messy code that I know how it needs to run (and fail) - it gives me the chance to notice various regressions. The patch you submitted does indeed cause a regression.

Please see:

https://gist.github.com/1121030

Paste those three files onto your disc and load test.html. Try to highlight any of the nested iframes and see if that works. It does not work for me.

If you have questions and would like me to help with something, please let me know.

Looking forward for the updated patch. Thank you!

::: browser/base/content/inspector.js
@@ +267,5 @@
> +      // the parent iframe position and its offset (borders and padding)
> +      if (frameWin.frameElement) {
> +        frameRect = frameWin.frameElement.getBoundingClientRect();
> +
> +        [offsetTop, offsetLeft] =

Please do let [foo1, foo2] here. These new variables currently end up in the global.

@@ +1115,5 @@
>          case "iframe":
>            let rect = node.getBoundingClientRect();
> +
> +          // gap between the iframe and its content window
> +          [offsetTop, offsetLeft] = this.getIframeContentOffset(node);

Same as above.

@@ +1142,5 @@
>    /**
> +   * Returns iframe content offset (iframe border + padding)
> +   * @param aIframe
> +   *        The iframe.
> +   * @returns [offsetTop, offsetLeft]

@returns array

Please explain the two array elements.

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v1.1

Thank you for the review Mihai! I made a stupid mistake in the last patch (moved a piece of code and thought it won't change anything). Fixed now :) Results should be better.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 550230 [details] [diff] [review]
patch v1.1

This is much better now.

Still, on the infamous test from...

http://mihaisucan.github.com/mozilla-work/test.html

I found a problem:

1. open the page and the inspector.
2. highlight "big div" from the nested iframe.
3. scroll with the mouse wheel or click to lock on the node, then scroll with the keyboard.

You will see that the iframe scrolls and the big div goes at the top outside of the view, still the highlighter goes outside of the iframe, which is wrong.

Again, this is a problem of code order. I think the clipping there is not correct.

Please add comments to each operation that's done there, to explain what happens and why. This would allow us to properly remember what we did here, when we get back to this code in a few months or whenever we do.

Thanks for the fixes!

(please note that you have asked me for sr, but I am not a superreviewer :) and this code does not need sr.)

Comment 7

[Paul Rouget [:paul]]

Attachment: patch v1.2

I think this time I got it right :)

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 551073 [details] [diff] [review]
patch v1.2

Review of attachment 551073 [details] [diff] [review]:
-----------------------------------------------------------------

This works fine now, you fixed the bugs. Yay!

Below are mainly nits. You have r+ once the comments are addressed, and you can ask a browser peer for review. Please make sure you send the patch to the try server as well.

Thank you!

::: browser/base/content/inspector.js
@@ +253,3 @@
>      let clientRect = this.node.getBoundingClientRect();
>  
> +    // Go up in the tree of frames to determine the correct rectangle

I think you want a period at the end of the sentence. The next comment line is independent.

@@ +262,1 @@
>      // coordinates and size.

This is an orphaned comment now.

@@ +264,2 @@
>  
> +    // We itereate through all the parent windows.

We iterate

@@ +291,5 @@
>  
> +      // Selection has been clipped to fit in its own window.
> +
> +      // Are we in the top-level window?
> +      if (frameWin.parent === frameWin) {

if (frameWin.parent === frameWin || !frameWin.frameElement) {

@@ +297,5 @@
>        }
>  
> +      // We are in an iframe.
> +      // We take into account the parent iframe position and its
> +      // offset (borders and padding)

Please add a period.

@@ +298,5 @@
>  
> +      // We are in an iframe.
> +      // We take into account the parent iframe position and its
> +      // offset (borders and padding)
> +      frameRect = frameWin.frameElement.getBoundingClientRect();

let frameRect.

@@ +1146,5 @@
> +          aX -= rect.left + offsetTop;
> +          aY -= rect.top + offsetLeft;
> +
> +          if (aX < 0 || aY < 0) {
> +            // Didn't reach the contentdocument, still over the iframe

The two comments are not consistent. The first comment does not start with a capital letter, while the second comment does so.

Please use a capital letter at the start of each comment and end with a period.

Also, "contentdocument" should be "contentDocument" or "content document".

@@ +1171,5 @@
> +   * @returns array [offsetTop, offsetLeft]
> +   *  offsetTop is the distance from the top of the iframe and the
> +   *    top of the content document.
> +   *  offsetLeft is the distance from the left of the iframe and the
> +   *    left of the content document.

Please indent the comment content such that offset* starts where array starts:

   * @returns array [offsetTop, offsetLeft]
   *          offsetTop is the distance from the top of the iframe and the

...

::: browser/base/content/test/inspector/browser_inspector_bug_674871.js
@@ +41,5 @@
> +      Services.obs.addObserver(isTheIframeSelected,
> +        INSPECTOR_NOTIFICATIONS.HIGHLIGHTING, false);
> +
> +      moveMouseOver(iframeNode, 1, 1);
> +      //InspectorUI.inspectNode(objectNode);

Please remove this line, given it's not needed.

@@ +69,5 @@
> +      INSPECTOR_NOTIFICATIONS.HIGHLIGHTING);
> +
> +    is(InspectorUI.selection, iframeBodyNode, "selection matches node");
> +    // 184 == 200 + 11(border) + 13(padding) - 40(scroll)
> +    ok(InspectorUI.highlighter._highlightRect.height == 184, "selection ");

You can do:

is(InspectorUI.highlighter._highlightRect.height, 184,
   "highlighter height");

Comment 9

[Rob Campbell [:rc] (:robcee)]

since paul's going offline I can address your review comments and send it over for a browser peer review. Thanks, Mihai!

Comment 10

[Rob Campbell [:rc] (:robcee)]

Attachment: patch v1.3

updated based on mihai's review comments. Forwarding to browser peer review.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 552102 [details] [diff] [review]
patch v1.3

You shouldn't need to explicitly adjust for padding/border if you're using getBoundingClientRect() - you just need to climb up to the root frame and adjust offsets as needed. If that isn't working something else wacky is going on.

Mobile's elementFromPoint is worth using as inspiration (its instanceof checks are better than the localName checks in this code):
http://hg.mozilla.org/mozilla-central/annotate/982a5835fba1/mobile/chrome/content/content.js#l166

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: patch v1.4

(In reply to Gavin Sharp from comment #11)
> Comment on attachment 552102 [details] [diff] [review]
> patch v1.3
> 
> You shouldn't need to explicitly adjust for padding/border if you're using
> getBoundingClientRect() - you just need to climb up to the root frame and
> adjust offsets as needed. If that isn't working something else wacky is
> going on.
> 
> Mobile's elementFromPoint is worth using as inspiration (its instanceof
> checks are better than the localName checks in this code):
> http://hg.mozilla.org/mozilla-central/annotate/982a5835fba1/mobile/chrome/
> content/content.js#l166

Rebased the patch, used the instanceof checks suggested above in elementFromPoint() and fixed the coordinate calculation in there, where offsetLeft and offsetTop had been swapped in the previous patch.

Still, I couldn't find a way to fix this bug without Paul's elaborate border/padding calculations, so that part remains.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

The frame's padding/border shouldn't matter, because you shouldn't be positioning anything in relation to the frame's visible edge (but rather to its viewport's edge, and its parent's, and so on). There must be some mismatch of expectations here, and I don't understand what they are because I'm not sure what exactly this code is trying to do. Is the highlighter just trying to highlight the element's bounding client rect, or is it more complicated than that?

Comment 14

[Paul Rouget [:paul]]

(In reply to Gavin Sharp from comment #13)
> The frame's padding/border shouldn't matter, because you shouldn't be
> positioning anything in relation to the frame's visible edge (but rather to
> its viewport's edge, and its parent's, and so on). There must be some
> mismatch of expectations here, and I don't understand what they are because
> I'm not sure what exactly this code is trying to do. Is the highlighter just
> trying to highlight the element's bounding client rect, or is it more
> complicated than that?

No, that's all. And I'm not sure to understand what you're saying. For example, here is what this code would do:

Let's say the bounding box of an element in an iframe is {top:10;left:10},
and the bounding box of the iframe is {top:33;left:33}.

The position of the element relatively to the parent document is 10 + 33 + iframe borders and padding.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gavin Sharp from comment #13)
> The frame's padding/border shouldn't matter, because you shouldn't be
> positioning anything in relation to the frame's visible edge (but rather to
> its viewport's edge, and its parent's, and so on). There must be some
> mismatch of expectations here, and I don't understand what they are because
> I'm not sure what exactly this code is trying to do. Is the highlighter just
> trying to highlight the element's bounding client rect, or is it more
> complicated than that?

As Paul explained, it's just that. The test I've been using is trying to get the highlighter to select the red area in this page (slightly modified version of the one in the mochitest):

data:text/html,<style>iframe{height:200px;border-width:%2011px%202px%2020px%2040px;border-style:%20solid;border-color:%20black;padding:%2013px%204px%2022px%2042px;}body,iframe{margin:0}</style><body><iframe%20src='data:text/html,<style>body{margin:0;height:100%;background-color:red}</style><body></body>'></iframe></body>

As I understand it the problem stems from the fact that element.getBoundingClientRect().top (or left) returns 0 for the element inside the styled iframe. In other words, the offsets are relative to the container's content box, not its border box (to use the terminology in http://msdn.microsoft.com/en-us/library/ms530302%28VS.85%29.aspx). I've seen an explicit mention of having to subtract the frame border when using getBoundingClientRect() here:

http://weblogs.asp.net/bleroy/archive/2008/01/29/getting-absolute-coordinates-from-a-dom-element.aspx

I tried to derive the border and padding size like this:
(iframe.getBoundingClientRect().width - node.getBoundingClientRect().width)/2, since the width and height contain useful values, but this only works when the left and right padding and border are equal.

Another idea I've tried was to get the border size from iframe.clientTop, but that leaves us with the padding, and I don't know of any way to get to that. But even if we could devise a way to calculate the necessary offset using all of iframe.getBoundingClientRect(), iframe.clientTop, node.getBoundingClientRect(), etc., I still think that Paul's version would be more straightforward to write and understand. As long as there is no performance implication from accessing style properties instead of layout offsets, I think this is a nice approach overall.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

OK, I see the issue now - there's no easy way to get the offset between the iframe's content (i.e. the origin for its children's bounding client rects) and its bounding client rect. I wonder whether we could add a way to do that that doesn't involve getting the computed style.

Comment 17

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

We really need a new API here that lets you get the boxes of one element relative to another. You'll need this to deal with transforms anyway.

Comment 18

[Paul Rouget [:paul]]

Does it mean we have to wait for this API to get this bug fixed? Can we use this workaround for now?

Comment 19

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

Yeah I think so

Comment 20

[Paul Rouget [:paul]]

Roc, which question are you answering?

Comment 21

[Rob Campbell [:rc] (:robcee)]

do we have a bug on file to create that new API?

We'd also like to have access to post-transformed coordinates as well, if we're spinning new API.

Also, I'd like to get the work-around in while this is under construction. Gavin, what do you think (provided we file the appropriate follow-ups and dependent bugs to get this fixed after we get our glossy new API).

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I logged Bug 626359 - Expose functionality to retrieve detailed post-transform geometry a while back. It should allow us to get the real transformed positions of objects using getQuads().

Comment 23

[Kyle Simpson]

(In reply to Michael Ratcliffe from comment #22)
> It should allow us to get the real transformed positions

By "real transformed positions", do we mean that we can tell the screen positions/dimensions of objects after page-zoom as well?

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (robcee) from comment #21)
> do we have a bug on file to create that new API?

Filed bug 684869 for that.

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Kyle Simpson from comment #23)
> (In reply to Michael Ratcliffe from comment #22)
> > It should allow us to get the real transformed positions
> 
> By "real transformed positions", do we mean that we can tell the screen
> positions/dimensions of objects after page-zoom as well?

The implementation would be element.getQuads(relativeTo) which returns a
list of Quads in CSS pixels relative to the top-left of relativeTo's first
CSS box's border-box (or possibly content-box) ... so it will not return the screen position.

Comment 26

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

(In reply to Paul Rouget [:paul] from comment #20)
> Roc, which question are you answering?

"Yes, use your workaround for now."

Comment 27

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

If we implement getQuads, then you'll be able to get coordinates relative to the root element or the viewport, and then you can use window.screenX/Y to get coordinates relative to the screen. But they'll be in CSS pixels relative to the element you ask about, which are "coordinates before zoom".

What do you actually do with these coordinates? i.e., how does the highlighter actually highlight things? I had assumed that the highlighter would insert something into the chrome document for the browser window, above the content document you're inspecting.

Comment 28

[Kyle Simpson]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> If we implement getQuads, then you'll be able to get coordinates relative to
> the root element or the viewport, and then you can use window.screenX/Y to
> get coordinates relative to the screen. But they'll be in CSS pixels
> relative to the element you ask about, which are "coordinates before zoom".
> 
> What do you actually do with these coordinates? i.e., how does the
> highlighter actually highlight things? I had assumed that the highlighter
> would insert something into the chrome document for the browser window,
> above the content document you're inspecting.

OK, I misspoke. I didn't mean that I needed "screen relative" coordinates. What I meant was, I needed absolute screen-rendered pixel sizes/dimensions.

The way the page-zoom works, you draw an element as 10px wide, but at 2x, it actually gets drawn at 20px wide. But there's no way to get that 20px rendered size directly. All the `offsetWidth` and `getBoundingClientRect()` and such APIs all return coordinates pre-zoom. So I was asking if this API being discusses which was going to return post-transform coordinates would give us post-zoom coordinates.

The way the highlighter works is that it creates a separate, non-zoomed, overlay "window" on top of the actual content window. So the content window is zoomed, and the highlighter window is not, and to get the highlighter to handle zoom correctly, we have to artificially change all our coordinates by the zoom factor. This works fine. 

But then, not having an API to test the real post-zoom coordinates against, the test for this feature is rather moot/unhelpful, because we essentially say: width*zoom == width*zoom. That's not testing the code, it's only testing the math engine in JavaScript. Having an independent way to verify that an element of size 10px is actually drawn 20px, and matching that 20px to the real non-zoomed 20px that I drew in the highlighter, would be a solid test.

Comment 29

[Rob Campbell [:rc] (:robcee)]

can we please not morph this bug into feature requests for layout? Kyle, please file a bug for the zoomed coordinates if you need one.

Thanks Robert. We'll use this work-around for now.

Comment 30

[Kyle Simpson]

(In reply to Rob Campbell [:rc] (robcee) from comment #29)
> can we please not morph this bug into feature requests for layout? Kyle,
> please file a bug for the zoomed coordinates if you need one.
> 
> Thanks Robert. We'll use this work-around for now.

I was not trying to morph this bug into anything. I was asking a question to clarify if the "post-transformation" coordinates being discussed here were also "post-zoom", or if that was something different.

Comment 31

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

(In reply to Kyle Simpson from comment #28)
> The way the highlighter works is that it creates a separate, non-zoomed,
> overlay "window" on top of the actual content window.

When you say "window" do you mean a DOM window or a real platform popup window (e.g. a XUL panel)?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

The highlighter seems to currently just use XUL boxes inserted into the chrome window (in a stack above the <browser>):

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/inspector.js#103

Comment 33

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

I was going to say, in that case, if we had getQuads, you could call element.getQuads(inspectorContainerElement) and you would get quads with the right dimensions for XUL boxes there.

But that won't work for multiprocess, where those elements aren't even in the same process.

But instead, you can get coordinates in the content document and use nsIDOMWindowUtils::screenPixelsPerCSSPixel to convert its CSS pixels to screen pixels.

Comment 34

[Kyle Simpson]

(In reply to comment #31 comment #32 comment #33)

In comment #29, rob asked that I not hijack this thread to be about zoom. I really was not trying to hijack. I was just trying to clarify if getQuads() was going to solve the standing issue of the post-zoom coordinates, or if we in fact needed to ask for another API. I was asking that clarification in the spirit of robcee's own comment #21.

In any case, I'm going to move the rest of the "zoom" discussion over to Bug #668254 and respond to roc there. Again, apologies for the inadvertent "hijacking".

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 554652 [details] [diff] [review]
patch v1.4

Please add a comment and bug reference to getIframeContentOffset explaining how it shouldn't need to exist :)

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: [in-fx-team] Patch v1.5

Comment added, thanks!

Comment 37

[Mihai Sucan [:msucan]]

I'd like to land this.

Panos: does this patch have a successful try server run?

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

I just started one:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=46adada0b0a4

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

There were only a few unrelated oranges, so I think it's safe to land.

Comment 40

[Mihai Sucan [:msucan]]

Comment on attachment 560285 [details] [diff] [review]
[in-fx-team] Patch v1.5

Pushed:
https://hg.mozilla.org/integration/fx-team/rev/31153dfd4307

Comment 41

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/31153dfd4307

Comment 42

[Teodosia Pop]

Verified Fixed using Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1.