Closed Bug 560830 Opened 10 years ago Closed 9 years ago

Handle DOMMouseScroll events in highlighter panels (reticle)

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rcampbell, Assigned: julian.viereck)

References

Details

(Whiteboard: [kd4b4])

Attachments

(1 file, 2 obsolete files)

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.
Summary: Handle DOMMouseScroll events in highlighter panels → Handle DOMMouseScroll events in highlighter panels (reticle)
Assignee: nobody → jviereck
Whiteboard: [kd4b4]
Attached patch Patch (obsolete) — Splinter Review
Implements scrolling while inspecting + unit test.
Attachment #462076 - Flags: feedback?(mihai.sucan)
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.
Attachment #462076 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #462076 - Attachment is obsolete: true
Attachment #462342 - Flags: review?
Attachment #462342 - Flags: review? → review?(gavin.sharp)
Attachment #462342 - Flags: review?(gavin.sharp) → review?(dolske)
Attachment #462342 - Flags: review?(dolske) → review?(sdwilsh)
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?
Same as former attachment but useing element.ownerDocument.defaultView; now to get the window under the mouse. Thanks a lot Gavin!
Attachment #462342 - Attachment is obsolete: true
Attachment #463165 - Flags: review?(sdwilsh)
Attachment #462342 - Flags: review?(sdwilsh)
Attachment #463165 - Flags: review?(sdwilsh) → review+
Status: NEW → ASSIGNED
Whiteboard: [kd4b4] → [kd4b4][checkin-needed]
Keywords: checkin-needed
Whiteboard: [kd4b4][checkin-needed] → [kd4b4]
Comment on attachment 463165 [details] [diff] [review]
[checked-in] Patch improved on Gavin's feedback

http://hg.mozilla.org/mozilla-central/rev/814a9de8b245
Attachment #463165 - Attachment description: Patch improved on Gavin's feedback → [checked-in] Patch improved on Gavin's feedback
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.