703692 - Source Editor: add support for the focus/blur events

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Mihai Sucan [:msucan]]

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]]

can I take this bug?

Comment 2

[Mihai Sucan [:msucan]]

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]

Attachment: Proposed fix.

I mapped the two events from orion to the editor.

Comment 4

[Mihai Sucan [:msucan]]

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]

Attachment: Proposed fix and testcase.

Added the test for focus and blur events. Thanks Mihai for all the help!

Comment 6

[Mihai Sucan [:msucan]]

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]

Attachment: Fixed tests.

Corrected tests as per Mihai's suggestions.

Comment 8

[Mihai Sucan [:msucan]]

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]

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

Improved test logic.

Comment 10

[Mihai Sucan [:msucan]]

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]]

This is ready to land!

Comment 12

[Mihai Sucan [:msucan]]

We need documentation for these new events.

Comment 13

[Mihai Sucan [:msucan]]

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] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4de94318cb24

Comment 15

[Eric Shepherd [:sheppy]]

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.