Last Comment Bug 725388 - Expose the Orion mouse events in the Source Editor
: Expose the Orion mouse events in the Source Editor
Status: RESOLVED FIXED
[sourceeditor][good first bug][mentor...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Spyros Livathinos
:
:
Mentors:
Depends on:
Blocks: 725235 725392
  Show dependency treegraph
 
Reported: 2012-02-08 09:54 PST by Mihai Sucan [:msucan]
Modified: 2012-02-17 17:11 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix with testcase. (5.81 KB, patch)
2012-02-13 02:04 PST, Spyros Livathinos
no flags Details | Diff | Splinter Review
Proposed fix with testcase. (5.81 KB, patch)
2012-02-13 04:53 PST, Spyros Livathinos
no flags Details | Diff | Splinter Review
Proposed fix with testcase. (5.68 KB, patch)
2012-02-15 02:49 PST, Spyros Livathinos
mihai.sucan: feedback+
Details | Diff | Splinter Review
Proposed fix with testcase. (5.95 KB, patch)
2012-02-15 11:40 PST, Spyros Livathinos
mihai.sucan: feedback+
Details | Diff | Splinter Review
[in-fx-team] Proposed fix with testcase. (5.95 KB, patch)
2012-02-15 12:27 PST, Spyros Livathinos
mihai.sucan: review+
Details | Diff | Splinter Review

Description Mihai Sucan [:msucan] 2012-02-08 09:54:25 PST
We need to allow listeners for mouseover, mousemove and mouseout in the Source Editor.
Comment 1 Spyros Livathinos 2012-02-13 02:04:05 PST
Created attachment 596621 [details] [diff] [review]
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 2012-02-13 04:53:36 PST
Created attachment 596652 [details] [diff] [review]
Proposed fix with testcase.

Wrong test on the previous attachment.
Comment 3 Mihai Sucan [:msucan] 2012-02-13 11:11:33 PST
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] 2012-02-13 11:22:54 PST
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 2012-02-15 02:49:52 PST
Created attachment 597352 [details] [diff] [review]
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] 2012-02-15 11:24:01 PST
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 2012-02-15 11:40:35 PST
Created attachment 597503 [details] [diff] [review]
Proposed fix with testcase.

Updated patch, fixed mouse event description and indentation issues.
Comment 8 Mihai Sucan [:msucan] 2012-02-15 12:19:44 PST
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 2012-02-15 12:27:36 PST
Created attachment 597522 [details] [diff] [review]
[in-fx-team] Proposed fix with testcase.

Fixed...
Comment 10 Mihai Sucan [:msucan] 2012-02-15 12:59:17 PST
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] 2012-02-15 13:00:11 PST
This is ready to land!
Comment 12 Mihai Sucan [:msucan] 2012-02-17 08:56:37 PST
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] 2012-02-17 08:57:18 PST
Spyros: thanks a lot for your awesome contributions!
Comment 14 Tim Taubert [:ttaubert] 2012-02-17 17:11:01 PST
https://hg.mozilla.org/mozilla-central/rev/b099928aaa34

Note You need to log in before you can comment on or make changes to this bug.