Closed
Bug 703692
Opened 13 years ago
Closed 13 years ago
Source Editor: add support for the focus/blur events
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 13
People
(Reporter: msucan, Assigned: livathinos.spyros)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js])
Attachments
(1 file, 3 obsolete files)
6.26 KB,
patch
|
msucan
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Whiteboard: [sourceeditor] → [sourceeditor][good first bug][mentor=msucan]
Updated•13 years ago
|
Whiteboard: [sourceeditor][good first bug][mentor=msucan] → [sourceeditor][good first bug][mentor=msucan][lang=js]
Updated•13 years ago
|
Priority: -- → P3
Comment 1•13 years ago
|
||
can I take this bug?
Reporter | ||
Comment 2•13 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•13 years ago
|
||
I mapped the two events from orion to the editor.
Attachment #595368 -
Flags: feedback?(mihai.sucan)
Reporter | ||
Comment 4•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
Corrected tests as per Mihai's suggestions.
Attachment #595386 -
Attachment is obsolete: true
Attachment #595426 -
Flags: review?(mihai.sucan)
Reporter | ||
Comment 8•13 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•13 years ago
|
||
Improved test logic.
Attachment #595426 -
Attachment is obsolete: true
Attachment #595443 -
Flags: review?(mihai.sucan)
Reporter | ||
Comment 10•13 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•13 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•13 years ago
|
||
We need documentation for these new events.
Blocks: 724962
Keywords: dev-doc-needed
Reporter | ||
Comment 13•13 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•13 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]
Comment 14•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 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
Comment 15•13 years ago
|
||
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
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•