Closed Bug 1059312 Opened 10 years ago Closed 8 years ago

The highlighter box is at the wrong position when switching to an iframe

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: ochameau, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

screenshot - The highlighter box is at the wrong position when switching to an iframe.png
13.86 KB, image/png
Details
bug1059312.v3.patch
6.18 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review 
When you switch toolbox context to an iframe and try to highlight a node,
the highlighter is displayed at the wrong position.
It doesn't take into account the position of the frame itself and displays the highlighter box at a position relative to top-level tab document,
instead of relative to the iframe document.
Probably comes from http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/LayoutHelpers.jsm#408
OS: Windows 7 → All
Hardware: x86_64 → All
It may be platform/environment specific. Or I do have something in my patch queue breaking the highlighter...
I'll try to confirm once frame patches reach nightly.

Here is my step to reproduce:
- open http://smartsearch.altervista.org/
- switch to the "to do list" iframe
- go to inspector and hover some nodes

The highlighter box appears on a wrong position for me.
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> It may be platform/environment specific. Or I do have something in my patch
> queue breaking the highlighter...
Probably, because these STR work for me.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > It may be platform/environment specific. Or I do have something in my patch
> > queue breaking the highlighter...
> Probably, because these STR work for me.

Hum. It really is broken. 
Easily reproducible in recent nightly when going to http://smartsearch.altervista.org/
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > It may be platform/environment specific. Or I do have something in my patch
> > queue breaking the highlighter...
> Probably, because these STR work for me.

Hum. It really is broken. 
Easily reproducible in recent nightly when going to http://smartsearch.altervista.org/ and switching to an iframe.
STR (with static testcase):
1. Open URL   data:text/html,<iframe src="data:text/html,<select style='margin:20px'>">
2. Open Inspector, switch to the iframe
3. Hover mouse over <select> element in that frame

Result:      Highlight ignores the iframe's position and therefore is displayed in wrong place
Expectation: Highlight should appear directly above the element which is "highlighted"
Hm, obviously I experienced different issue than the original one (should I file separate bug?), because the highlighter behavior was changed between 2014-11-06 and 2014-11-07:
> pushlog_url:   https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba
I think it's bug 985597
Before that, area was highlighted within <body>, and after those changes element is highlighted within <iframe>. Looks like that patch just changed the target document for highlight w/o fixing coordinates

I personally think that old behavior was better, because currently it's hard to determine node's position if iframe is out of view.
Blocks: 985597
Keywords: regression
When navigating to another iframe, the pointer to the current window is not updated in the AutoRefreshHighlighter.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached patch bug1059312.v1.patch (obsolete) — — Splinter Review 
Bug 1059312 - Fix highlighter offset after switching iframe context;r=pbro

The auto-refresh highlighter base class was getting a reference to the window
object to use for highlighting at instanciation time.
This reference should be updated if a navigation of the highlight environment
occurs.

This commit maps the "win" property to a getter, so that win is always in sync
with the current highlight environment.
Attachment #8714106 - Flags: review?(pbrosset)
Comment on attachment 8714106 [details] [diff] [review]
bug1059312.v1.patch

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

The code changes look good to me.
Please try adding the inspector-updated event in the test too.

::: devtools/client/inspector/test/browser_inspector_highlighter-iframes_02.js
@@ +54,5 @@
> +  frameBtns[frameIndex].click();
> +
> +  yield willNavigate;
> +  yield newRoot;
> +  info("Navigation to the iframe is done.");

This will cause a full update of the inspector, which means that sidebar panels, breadcrumbs and markup-view will all be updated. Everything happens asynchronously, so I'm suspecting that only waiting for will-navigate and new-root won't be enough to wait for all panels to update (they do further server-requests of their own), and we might end up with pending requests when the test ends which, in turn, may cause unhandled promise rejections, and test failures.
It might be good to also wait for inspector.once("inspector-updated") which is emitted only after everything else has been updated.
Attachment #8714106 - Flags: review?(pbrosset) → review+
Thanks for the review! As discussed on IRC, now only listening to "editor-updated".

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8764bb91b00a
Attachment #8714106 - Attachment is obsolete: true
Attachment #8714267 - Flags: review+
Looks like using only "editor-updated" fails when running in non e10s mode [1].

A first editor-updated event is triggered between will-navigate and new-root.
After receiving new-root however, another "editor-updated" event is fired. Locally, waiting on this event seems to work both on e10s and non-e10s.

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3caacbadefdb
Attachment #8714267 - Attachment is obsolete: true
Attachment #8714299 - Flags: review+
Of course, typos : 
- s/editor-updated/inspector-updated
- [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8764bb91b00a&selectedJob=16159665
Last try push looks good, please checkin attachment 8714299 [details] [diff] [review]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c06514269433
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: