Closed
Bug 1351081
Opened 7 years ago
Closed 6 years ago
Devtools incorrectly highlight elements in iframe when CSS 'transform' is applied
Categories
(DevTools :: Inspector, defect, P3)
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
Assignee | ||
Comment 2•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•7 years ago
|
Severity: normal → enhancement
Priority: -- → P3
Updated•7 years ago
|
Assignee: nobody → zer0
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
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
I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=127487782&repo=autoland https://hg.mozilla.org/integration/autoland/rev/530358f38866878c5bd86c0ac6801656de6aac62
Flags: needinfo?(zer0)
Comment 9•7 years ago
|
||
Thanks Wes! I'm looking into what's going on with the CSS shapes' tests.
Flags: needinfo?(zer0)
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=428dfc5a5548d35342a28579605da507933cf798
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8902235 [details] Bug 1351081 - added `relativeTo` to getBoxQuads; https://reviewboard.mozilla.org/r/173756/#review188664
Attachment #8902235 -
Flags: review?(pbrosset) → review+
Comment 14•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
(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
Comment 19•6 years ago
|
||
Patrick, do you think you can finish this?
Flags: needinfo?(gl) → needinfo?(pbrosset)
Comment hidden (mozreview-request) |
Comment 21•6 years ago
|
||
mozreview-review |
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]
Assignee | ||
Comment 22•6 years ago
|
||
Taking this over from Matteo. And fixing the test failures.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Assignee | ||
Updated•6 years ago
|
Attachment #8902235 -
Attachment is obsolete: true
Comment 23•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 24•6 years ago
|
||
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).
Comment hidden (mozreview-request) |
Comment 26•6 years ago
|
||
Pushed by pbrosset@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/65e0fefdab51 Always retrieve quads relatively to the top window, correctly; r=gl
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/65e0fefdab51
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•