Last Comment Bug 703692 - Source Editor: add support for the focus/blur events
: Source Editor: add support for the focus/blur events
Status: RESOLVED FIXED
[sourceeditor][good first bug][mentor...
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 13
Assigned To: Spyros Livathinos
:
Mentors:
Depends on:
Blocks: 724962
  Show dependency treegraph
 
Reported: 2011-11-18 12:06 PST by Mihai Sucan [:msucan]
Modified: 2012-04-28 14:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix. (1.65 KB, patch)
2012-02-08 03:25 PST, Spyros Livathinos
mihai.sucan: feedback+
Details | Diff | Splinter Review
Proposed fix and testcase. (6.05 KB, patch)
2012-02-08 05:15 PST, Spyros Livathinos
no flags Details | Diff | Splinter Review
Fixed tests. (6.25 KB, patch)
2012-02-08 08:53 PST, Spyros Livathinos
no flags Details | Diff | Splinter Review
[in-fx-team] Proposed fix with working test. (6.26 KB, patch)
2012-02-08 10:19 PST, Spyros Livathinos
mihai.sucan: review+
Details | Diff | Splinter Review

Description Mihai Sucan [:msucan] 2011-11-18 12:06:32 PST
Orion upstream has added support for the focus/blur events. We should expose these inside the Source Editor component.

See the upstream bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=354270
Comment 1 Leonard Camacho [:lcamacho] 2012-02-07 17:28:33 PST
can I take this bug?
Comment 2 Mihai Sucan [:msucan] 2012-02-08 01:24:17 PST
Leonard, I am sorry but Spyros has already started working on a patch yesterday. We can look into finding a different source editor bug, if you want? Please ping me on IRC.
Comment 3 Spyros Livathinos 2012-02-08 03:25:41 PST
Created attachment 595368 [details] [diff] [review]
Proposed fix.

I mapped the two events from orion to the editor.
Comment 4 Mihai Sucan [:msucan] 2012-02-08 04:40:19 PST
Comment on attachment 595368 [details] [diff] [review]
Proposed fix.

This looks good. Thank you Spyros for your contribution! You can also add your name to the contributors list at the top of the files.

Looking forward to the test!
Comment 5 Spyros Livathinos 2012-02-08 05:15:20 PST
Created attachment 595386 [details] [diff] [review]
Proposed fix and testcase.

Added the test for focus and blur events. Thanks Mihai for all the help!
Comment 6 Mihai Sucan [:msucan] 2012-02-08 05:56:56 PST
Comment on attachment 595386 [details] [diff] [review]
Proposed fix and testcase.

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

Spyros: this is awesome work! Thanks! I do have some comments below, to improve your test.

(I liked that all the tests pass and there were no issues with your patch!)

::: browser/devtools/sourceeditor/test/browser_bug703692_focus_blur.js
@@ +49,5 @@
> +  editor.focus();
> +  ok(event, "Focus event fired");
> +
> +  window.focus();
> +  ok(event, "Blur event fired.");

You need to clear the event object before you wait for the second event to fire.

Also, I suggest you do the following: use two event handlers, one for focus, one for blur. In the focus event handler you add the blur event listener, and in the blur event handler you finish the test. Also, remove the event listeners for focus and blur, in each event handler.
Comment 7 Spyros Livathinos 2012-02-08 08:53:17 PST
Created attachment 595426 [details] [diff] [review]
Fixed tests.

Corrected tests as per Mihai's suggestions.
Comment 8 Mihai Sucan [:msucan] 2012-02-08 09:47:37 PST
Comment on attachment 595426 [details] [diff] [review]
Fixed tests.

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

Thanks for your update! Almost there! Please look into the comments below.

::: browser/devtools/sourceeditor/test/browser_bug703692_focus_blur.js
@@ +45,5 @@
> +    ok(aEvent, "Focus event fired");
> +  };
> +
> +  let blurHandler = function(aEvent) {
> +    editor.removeEventListener(SourceEditor.EVENTS.BLUR, executeSoon(testEnd));

This doesn't remove the blur event listener. I think you mean:

editor.removeEventListener(SourceEditor.EVENTS.BLUR, blurHandler);

@@ +47,5 @@
> +
> +  let blurHandler = function(aEvent) {
> +    editor.removeEventListener(SourceEditor.EVENTS.BLUR, executeSoon(testEnd));
> +    
> +    ok(aEvent, "Blur event fired");

After this line call executeSoon(testEnd).

@@ +54,5 @@
> +  editor.addEventListener(SourceEditor.EVENTS.FOCUS, focusHandler);
> +
> +  editor.focus();
> +
> +  window.focus();

Please move this into the focusHandler(), after the ok() call.
Comment 9 Spyros Livathinos 2012-02-08 10:19:30 PST
Created attachment 595443 [details] [diff] [review]
[in-fx-team] Proposed fix with working test.

Improved test logic.
Comment 10 Mihai Sucan [:msucan] 2012-02-08 10:54:03 PST
Comment on attachment 595443 [details] [diff] [review]
[in-fx-team] Proposed fix with working test.

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

Thank you Spyros! This is a well executed contribution - well done for a first patch in the Source Editor.
Comment 11 Mihai Sucan [:msucan] 2012-02-08 10:54:40 PST
This is ready to land!
Comment 12 Mihai Sucan [:msucan] 2012-02-08 12:25:57 PST
We need documentation for these new events.
Comment 13 Mihai Sucan [:msucan] 2012-02-09 04:45:39 PST
Comment on attachment 595443 [details] [diff] [review]
[in-fx-team] Proposed fix with working test.

Landed:
https://hg.mozilla.org/integration/fx-team/rev/4de94318cb24
Comment 14 Tim Taubert [:ttaubert] 2012-02-10 07:26:20 PST
https://hg.mozilla.org/mozilla-central/rev/4de94318cb24
Comment 15 Eric Shepherd [:sheppy] 2012-04-28 14:52:39 PDT
These events were already documented but not tagged as being new in Firefox 13. Now they are:

https://developer.mozilla.org/en/JavaScript_code_modules/source-editor.jsm#Events

Listed on Firefox 13 for developers.

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