Source Editor: add support for the focus/blur events

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: msucan, Assigned: Spyros Livathinos)

Tracking

({dev-doc-complete})

Trunk
Firefox 13
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

Updated

5 years ago
Whiteboard: [sourceeditor] → [sourceeditor][good first bug][mentor=msucan]

Updated

5 years ago
Whiteboard: [sourceeditor][good first bug][mentor=msucan] → [sourceeditor][good first bug][mentor=msucan][lang=js]
Priority: -- → P3
can I take this bug?
(Reporter)

Comment 2

5 years ago
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.
Assignee: nobody → livathinos.spyros
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 595368 [details] [diff] [review]
Proposed fix.

I mapped the two events from orion to the editor.
Attachment #595368 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 4

5 years ago
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!
Attachment #595368 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 595386 [details] [diff] [review]
Proposed fix and testcase.

Added the test for focus and blur events. Thanks Mihai for all the help!
Attachment #595368 - Attachment is obsolete: true
Attachment #595386 - Flags: review?(mihai.sucan)
(Reporter)

Comment 6

5 years ago
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.
Attachment #595386 - Flags: review?(mihai.sucan)
(Assignee)

Comment 7

5 years ago
Created attachment 595426 [details] [diff] [review]
Fixed tests.

Corrected tests as per Mihai's suggestions.
Attachment #595386 - Attachment is obsolete: true
Attachment #595426 - Flags: review?(mihai.sucan)
(Reporter)

Comment 8

5 years ago
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.
Attachment #595426 - Flags: review?(mihai.sucan)
(Assignee)

Comment 9

5 years ago
Created attachment 595443 [details] [diff] [review]
[in-fx-team] Proposed fix with working test.

Improved test logic.
Attachment #595426 - Attachment is obsolete: true
Attachment #595443 - Flags: review?(mihai.sucan)
(Reporter)

Comment 10

5 years ago
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.
Attachment #595443 - Flags: review?(mihai.sucan) → review+
(Reporter)

Comment 11

5 years ago
This is ready to land!
Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js] → [sourceeditor][good first bug][mentor=msucan][lang=js][land-in-fx-team]
(Reporter)

Comment 12

5 years ago
We need documentation for these new events.
Blocks: 724962
Keywords: dev-doc-needed
(Reporter)

Comment 13

5 years ago
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
Attachment #595443 - Attachment description: Proposed fix with working test. → [in-fx-team] Proposed fix with working test.
(Reporter)

Updated

5 years ago
Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js][land-in-fx-team] → [sourceeditor][good first bug][mentor=msucan][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4de94318cb24
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js][fixed-in-fx-team] → [sourceeditor][good first bug][mentor=msucan][lang=js]
Target Milestone: --- → Firefox 13
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.