Closed Bug 1351081 Opened 3 years ago Closed 2 years ago

Devtools incorrectly highlight elements in iframe when CSS 'transform' is applied

Categories

(DevTools :: Inspector, defect, P3)

45 Branch
defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: 684sigma, Assigned: pbro)

References

Details

Attachments

(1 file, 1 obsolete file)

I have a problem with Firefox ESR 45. It also happens in Beta 52, Beta 53, Nightly 55.
Devtools (all of them) incorrectly highlight element in iframe, if CSS 'transform' is applied.
Here're 4 ways to reproduce the bug:


First:
1. Open attachment 8851780 [details]
2. Inspect <select> element in inspector

Result: blue highlight is twice as large as the element
Expected: blue highlight should cover the element and be of the same size


Second:
1. Open attachment 8851780 [details]
2. Inspect <select> element, log it into console
3. Hover mouse over the logged element

Result: blue highlight is twice as large as the element
Expected: blue highlight should cover the element and be of the same size


Third:
1. Open attachment 8851780 [details]
2. Inspect <select> element in inspector
3. In ruleview click button "highlight all elements matching this selector"

Result: blue highlight is twice as large as the element
Expected: blue highlight should cover the element and be of the same size


Fourth:
1. Open attachment 8851780 [details]
2. Open style editor, hover mouse over selector "select"

Result: gray highlight is twice as large as the element
Expected: the gray highlight should cover the element and be of the same size


The error is the same in every case, and I came to conclusion that all highlighters in devtools use the same broken function (or something like that), therefore I reported all these together. Please fix.
Another bug detected in this document is Bug 1351080.
FF33 has the bug too, I guess the issue is here since the implementation of the Inspector.
Component: Developer Tools → Developer Tools: Inspector
(In reply to 684sigma from comment #0)
> The error is the same in every case, and I came to conclusion that all
> highlighters in devtools use the same broken function (or something like
> that), therefore I reported all these together. Please fix.
> Another bug detected in this document is Bug 1351080.
Thank you for filing this issue. Yes you are right, all highlights go through the same mechanism, so they all suffer from this bug.

(In reply to Loic from comment #1)
> FF33 has the bug too, I guess the issue is here since the implementation of
> the Inspector.
It may very well be indeed.
My hunch is the following: to get the element's size, we use element.getBoxQuads(), but of course, that runs within the iframe document, so it probably returns the size relative to this document.
Now, the iframe is itself in a document that has a different scale transform applied, so really we should try and get the size of elements relative to the root document.

I think this is correct because if you open the test case provided and do this:
- select the <select> element in the inspector,
- open the console
- type the following: $0.getBoxQuads()[0].bounds.width
==> you get the erroneous ~117px

However if you do:
- still with the <select> element selected
- type this in the console: $0.getBoxQuads({relativeTo:$0.ownerDocument.defaultView.parent.document})[0].bounds.width
==> you now get ~58px

The place where we do this in the code is http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/devtools/shared/layout/utils.js#196-198

The fix should probably be adding the `relativeTo` property to the object parameter passed to `getBoxQuads` and make it point to the root document.
I think we can get the root document with `getTopWindow(getWindowFor(node)).document` or something like that.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → enhancement
Priority: -- → P3
Assignee: nobody → zer0
Note, I tried to write a unit test for this, unfortunately it's not easy, since both the method we use to check if the node is correctly highlighted (http://searchfox.org/mozilla-central/source/devtools/client/shared/test/test-actor.js#871) and the way we draw the box-model, are based on the same implementation – `getAdjustedQuads` – therefore it will always pass, no matter if the rendering is correct or not.
Duplicate of this bug: 1342975
Comment on attachment 8902235 [details]
Bug 1351081 - added `relativeTo` to getBoxQuads;

https://reviewboard.mozilla.org/r/173756/#review179910
Attachment #8902235 - Flags: review?(pbrosset) → review+
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a69de9c73c6
added `relativeTo` to getBoxQuads, improved scrolling with fixed nodes; r=pbro
Thanks Wes! I'm looking into what's going on with the CSS shapes' tests.
Flags: needinfo?(zer0)
Comment on attachment 8902235 [details]
Bug 1351081 - added `relativeTo` to getBoxQuads;

Hi Patrick, since the code is different from the first review, I'd like another quick round. I simplify a bit, also since the bug 1377879 helped with fixed elements.
Attachment #8902235 - Flags: review+ → review?(pbrosset)
Comment on attachment 8902235 [details]
Bug 1351081 - added `relativeTo` to getBoxQuads;

https://reviewboard.mozilla.org/r/173756/#review188664
Attachment #8902235 - Flags: review?(pbrosset) → review+
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d8711590ab0
added `relativeTo` to getBoxQuads; r=pbro
Backed this out for mac devtools failures like https://treeherder.mozilla.org/logviewer.html#?job_id=133387919&repo=autoland
Flags: needinfo?(zer0)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f4e69a521a57
Backed out changeset 0d8711590ab0 for mac dt bustage in browser_inspector_highlighter-iframes_01.js a=backout
(In reply to Wes Kocher (:KWierso) from comment #15)
> Backed this out for mac devtools failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=133387919&repo=autoland

I didn't catch at first because it seemed an intermittent, thanks Wes! 

Apparently the issue comes from a difference in how the method `getRect`, used in tests, and the `getBoxQuads` with `relativeTo` are working. In some cases, we have a marginal difference in the results, that causes the problem:

[461.3333282470703,10.199996948242188]
[461.33331298828125,10.199999809265137]

A quick fix could be truncate during the test comparison to four digits after the decimal point (we have done similar things). 
But I believe it would be properly better fixing both the layout/utils method and the tests. 

First of all, `testActor.assertHighlightedNode`[1] relies on `getRect` but this is wrong: if an element has any transformation applied, `getRect` would returns the untrasformed points. Therefore it would be better obtain the quads and match them. And we're already doing so, in `testActor.isNodeCorrectlyHighlighted`; therefore if I'm not missing something, we could just remove `assertHighlightedNode` and use `isNodeCorrectlyHighlighted` instead.

I would also file a follow up bug to remove `getRect`, since it doesn't seems to have much value to me, we have two places where it used, the test and the screenshot command[3], and in both cases we should use quads instead. And there is still `getNodeBounds`[4] method as alternative for untransformed bounds.

[1] http://searchfox.org/mozilla-central/source/devtools/client/shared/test/test-actor.js#934
[2] http://searchfox.org/mozilla-central/source/devtools/client/shared/test/test-actor.js#871
[3] http://searchfox.org/mozilla-central/source/devtools/shared/gcli/commands/screenshot.js#295
[4] http://searchfox.org/mozilla-central/source/devtools/shared/layout/utils.js#380
Flags: needinfo?(zer0)
Gabriel, do you have anyone in mind that could finish this up?  It seems a few test adjustments are all that's left.
Flags: needinfo?(gl)
Assignee: zer0 → nobody
Patrick, do you think you can finish this?
Flags: needinfo?(gl) → needinfo?(pbrosset)
Comment on attachment 8963896 [details]
Bug 1351081 - Always retrieve quads relatively to the top window, correctly;

https://reviewboard.mozilla.org/r/232748/#review238170


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/shared/test/test-actor.js:1108
(Diff revision 1)
>    }
>  
> +  // Reduce the length of the fractional part because this is likely to cause errors when
> +  // the point is on the edge of the polygon.
> +  point = point.map(n => n.toFixed(2));
> +  polygon = polygon.map(point => point.map(n => n.toFixed(2)));

Error: 'point' is already declared in the upper scope. [eslint: no-shadow]
Taking this over from Matteo. And fixing the test failures.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Attachment #8902235 - Attachment is obsolete: true
Comment on attachment 8963896 [details]
Bug 1351081 - Always retrieve quads relatively to the top window, correctly;

https://reviewboard.mozilla.org/r/232748/#review238672
Attachment #8963896 - Flags: review?(gl) → review+
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d754014601df631bebc2746278915a1cd8aadb9
A bunch of unrelated intermittents. And an ESLint failure that I've fixed locally (will push the new commit soon).
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65e0fefdab51
Always retrieve quads relatively to the top window, correctly; r=gl
https://hg.mozilla.org/mozilla-central/rev/65e0fefdab51
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.