Handle DOMMouseScroll events in highlighter panels (reticle)

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: rc, Assigned: Julian Viereck)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kd4b4])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Summary: Handle DOMMouseScroll events in highlighter panels → Handle DOMMouseScroll events in highlighter panels (reticle)

Updated

8 years ago
Assignee: nobody → jviereck

Updated

8 years ago
Whiteboard: [kd4b4]
(Assignee)

Comment 1

8 years ago
Created attachment 462076 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 3

8 years ago
Created attachment 462342 [details] [diff] [review]
Patch improved based on Mihai's feedback
Attachment #462076 - Attachment is obsolete: true
Attachment #462342 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #462342 - Flags: review? → review?(gavin.sharp)
(Assignee)

Updated

8 years ago
Attachment #462342 - Flags: review?(gavin.sharp) → review?(dolske)
(Assignee)

Updated

8 years ago
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?
(Assignee)

Comment 5

8 years ago
Created attachment 463165 [details] [diff] [review]
[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!
Attachment #462342 - Attachment is obsolete: true
Attachment #463165 - Flags: review?(sdwilsh)
Attachment #462342 - Flags: review?(sdwilsh)
Attachment #463165 - Flags: review?(sdwilsh) → review+
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: [kd4b4] → [kd4b4][checkin-needed]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [kd4b4][checkin-needed] → [kd4b4]
(Reporter)

Comment 6

8 years ago
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
(Reporter)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.