560830 - Handle DOMMouseScroll events in highlighter panels (reticle)

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

Currently, when highlighting with the mouse inside a highlighter panel, the scroll wheel does nothing. The document should scroll when a user moves the wheel or uses a scrolling gesture.

Comment 1

[Julian Viereck]

Attachment: Patch

Implements scrolling while inspecting + unit test.

Comment 2

[Mihai Sucan [:msucan]]

Comment on attachment 462076 [details] [diff] [review]
Patch

Hello Julian!

This is my patch feedback.


In inspector.js:

+    if (aEvent.axis == aEvent.HORIZONTAL_AXIS) {
+      win.scrollBy(aEvent.detail, 0);
+    }
+    else {
+      win.scrollBy(0, aEvent.detail);
+    }

The inspector.js code uses "} else {" without \n, and this is recommended by the 
MDN style guide actually.


In the test file:

+  content.location = "data:text/html,iframe tests for inspector";

I believe you should say "mouse scroll tests". The bug fix you implemented is 
related not only to frames, it also makes mouse scroll to work in normal pages 
without any frames when the panel overlay is on top.


Again, in the test file:
+function runIframeTests(aEvt)
+function performScrollingTest(aEvt)

Why not use aEvent?

Also, the test code uses a mix of single and double quotes. I believe you should 
use only double quotes.


+    let iFrameDoc = iframe.contentDocument;

I think that should be consistent with the rest of the naming used. Maybe 
iframedoc? Or iframeDoc.


+  is (iframe.contentDocument.body.scrollTop, 50, "inspected iframe scrolled");

I think you should make that "is(" instead of "is (".

That's about all. I really like your fix and test code! You get feedback+ from 
me with the changes I suggested.

Comment 3

[Julian Viereck]

Attachment: Patch improved based on Mihai's feedback

Comment 4

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

Comment on attachment 462342 [details] [diff] [review]
Patch improved based on Mihai's feedback

>diff --git a/browser/base/content/inspector.js b/browser/base/content/inspector.js

>+  windowFromPoint: function IUI_windowFromPoint(aWindow, aX, aY)

Can't you just use elementFromPoint().ownerDocument.defaultView instead?

Comment 5

[Julian Viereck]

Attachment: [checked-in] Patch improved on Gavin's feedback

Same as former attachment but useing element.ownerDocument.defaultView; now to get the window under the mouse. Thanks a lot Gavin!

Comment 6

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

Comment on attachment 463165 [details] [diff] [review]
[checked-in] Patch improved on Gavin's feedback

http://hg.mozilla.org/mozilla-central/rev/814a9de8b245