725388 - Expose the Orion mouse events in the Source Editor

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Mihai Sucan [:msucan]]

We need to allow listeners for mouseover, mousemove and mouseout in the Source Editor.

Comment 1

[Spyros Livathinos]

Attachment: Proposed fix with testcase.

I added the three events from Orion (MouseOver, MouseOut, MouseMove) and mapped them to the Source Editor's known events collection.

Comment 2

[Spyros Livathinos]

Attachment: Proposed fix with testcase.

Wrong test on the previous attachment.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 596652 [details] [diff] [review]
Proposed fix with testcase.

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

This is awesome!  Thanks a lot for your patch Spyros! This is looking good, but please address the comments below.

Looking forward for the updated patch!

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +167,5 @@
> +   * annotation. The event object properties:
> +   *
> +   *   - lineIndex - the line index of the annotation under the pointer.
> +   *   - e - the mouse move event.
> +   */

This comments are not the right ones. In orion.js you saw the onMouseOver/Move/Out methods of the Ruler object. Search for the onMouseMove of the TextView object, see line 3747. The actual event objects are in the _createMouseEvent() method of the TextView object: event, x and y. The event property is the actual DOM mouse event, and the x and y coordinates are the "document coordinates" - these are absolute pixel coordinates of the mouse event where it happened, irrespective of the view scroll. So, in bug 725392 we will add a method to convert these coordinates to character offsets in the document.

::: browser/devtools/sourceeditor/test/browser_bug725388_mouse_events.js
@@ +39,5 @@
> +{
> +  let mMoveHandler = function(aEvent) {
> +    editor.removeEventListener(SourceEditor.EVENTS.MOUSE_MOVE, mMoveHandler);
> +
> +    ok(aEvent, "MouseMove event fired.");

Please check in each event handler that aEvent.event.type is what we expect: mousemove, mouseout and mouseover.

@@ +60,5 @@
> +
> +  let text = "BrowserBug - 725388";
> +  editor.setText(text);
> +
> +  let target = view._clientDiv;

Please use the editor.editorElement object as target. Tests should not rely on editor._view, nor view._clientDiv - these are private properties. We use these in very few tests under strict rules: that is, if things can't be tested otherwise (for example the drag and drop tests do this). Please let me know if you can't get this to work without relying on private properties. Thanks!

@@ +65,5 @@
> +  let targetWin = target.ownerDocument.defaultView;
> +
> +  editor.addEventListener(SourceEditor.EVENTS.MOUSE_MOVE, mMoveHandler);
> +  editor.addEventListener(SourceEditor.EVENTS.MOUSE_OVER, mOverHandler);
> +  editor.addEventListener(SourceEditor.EVENTS.MOUSE_OUT, mOutHandler);

Please do these tests in sequence, or add a counter-like function that tracks that all these events fired, so you can end the test only when all of these events fired. Currently the test ends prematurely if mouseout happens first. Thanks!

Comment 4

[Mihai Sucan [:msucan]]

I also noticed the patch has trailing whitespaces. Please make sure patches do not have trailing whitespaces. hg qdiff/diff with colors enabled shows if you have such whitespace in red.

Comment 5

[Spyros Livathinos]

Attachment: Proposed fix with testcase.

Made changes according to Mihai's comments (removed trailing whitespace,
broke the test into separate steps etc.)

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 597352 [details] [diff] [review]
Proposed fix with testcase.

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

Thanks for your updated patch Spyros! This is almost ready to land. Please update it based on the comments below.

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +164,5 @@
> +
> +  /**
> +   * The MouseMove event is sent when the user moves the mouse over a line
> +   * annotation. The event object properties:
> +   *   - e - the mouse move event.

The list of object properties needs to be:

- event - the DOM mousemove event object.
- x and y - the mouse coordinates relative to the document being edited.

(same for all of the three mouse events)

::: browser/devtools/sourceeditor/test/browser_bug725388_mouse_events.js
@@ +78,5 @@
> +
> +  editor.focus();
> +  waitForFocus(function() {
> +  EventUtils.synthesizeMouse(target, 1, 1, {type: "mousemove"},
> +                               targetWin);

nit: indentation problems.

Comment 7

[Spyros Livathinos]

Attachment: Proposed fix with testcase.

Updated patch, fixed mouse event description and indentation issues.

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 597503 [details] [diff] [review]
Proposed fix with testcase.

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

Thanks for the quick update!

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +172,5 @@
> +
> +  /**
> +   * The MouseOver event is sent when the mouse pointer enters a line
> +   * annotation. The event object properties:
> +   *   - event - the DOM mousemove event object.

This should be "mouseover".

@@ +180,5 @@
> +
> +  /**
> +   * This MouseOut event is sent when the mouse pointer exits a line
> +   * annotation. The event object properties:
> +   *   - event - the DOM mousemove event object.

This should be "mouseout".

Comment 9

[Spyros Livathinos]

Attachment: [in-fx-team] Proposed fix with testcase.

Fixed...

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 597522 [details] [diff] [review]
[in-fx-team] Proposed fix with testcase.

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

Thanks for your contribution! Awesome work!

Comment 11

[Mihai Sucan [:msucan]]

This is ready to land!

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 597522 [details] [diff] [review]
[in-fx-team] Proposed fix with testcase.

Landed:
https://hg.mozilla.org/integration/fx-team/rev/b099928aaa34

Comment 13

[Mihai Sucan [:msucan]]

Spyros: thanks a lot for your awesome contributions!

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b099928aaa34