The default bug view has changed. See this FAQ.

Orion source editor should have built-in context menu

RESOLVED FIXED in Firefox 13

Status

()

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

People

(Reporter: Jason Barnabe (np), Assigned: msucan)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
Firefox 13
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
I'm using the Orion source editor in my extension Stylish. I think it would be useful if Orion came with a built-in context menu for actions like undo, redo, copy, paste, etc. This context menu could be overlayed to include window-specific items or even replaced with a whole other menu. This would eliminate the need for me (or anyone else) to implement my own context code.

Comment 1

6 years ago
Problem is on Windows 7 also.
You can create a context menu for the editor, but agree that it would be nice to provide a default.

see, http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#718

for an example of adding a context menu handler.
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 3

6 years ago
This is something we can solve in Source Editor code, to allow users of the JSM to provide a custom context menu, both for the Orion and textarea components. (not just for orion)
Whiteboard: [sourceeditor]

Comment 4

6 years ago
I have some related comments.

Using scratchpad thru Tools/Web Development/Scratchpad when pasting code into the window the code does not get line numbers assigned.

However if you paste the same code to a text editor such as notepad then copy from notepad and paste to scratchpad, it works fine.

Before line numbers are assigned you cannot touch the code - it gets deleted from scratchpad.
(Assignee)

Comment 5

6 years ago
(In reply to Bruce A. Wittmeier from comment #4)
> I have some related comments.
> 
> Using scratchpad thru Tools/Web Development/Scratchpad when pasting code
> into the window the code does not get line numbers assigned.
> 
> However if you paste the same code to a text editor such as notepad then
> copy from notepad and paste to scratchpad, it works fine.
> 
> Before line numbers are assigned you cannot touch the code - it gets deleted
> from scratchpad.

This is a known separate bug, see bug 684862. Should be fixed in the latest Firefox nightlies.

Comment 6

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P3

Updated

5 years ago
Priority: P3 → P2
(Assignee)

Updated

5 years ago
Assignee: nobody → mihai.sucan
Version: unspecified → Trunk
(Assignee)

Updated

5 years ago
Depends on: 700893
(Assignee)

Comment 7

5 years ago
Created attachment 593591 [details] [diff] [review]
proposed patch

Proposed patch. This adds a default context menu to the Source Editor and it makes the Style Editor changes needed to be able to benefit from the new feature. Also updated Scratchpad accordingly, to use the new context menu config from the Source Editor.

Looking forward to your review! Thanks!
Attachment #593591 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Blocks: 710925
(Assignee)

Comment 8

5 years ago
Created attachment 595816 [details] [diff] [review]
rebased patch
Attachment #593591 - Attachment is obsolete: true
Attachment #593591 - Flags: review?(rcampbell)
Attachment #595816 - Flags: review?(rcampbell)
(Assignee)

Comment 9

5 years ago
The new SourceEditor initialization option, contextMenu, needs to be documented, please see the patch. Also the new overlay features: the #sourceEditorContextMenu menupopup and the two new se-cmd-undo and se-cmd-redo commands. Usage examples are in the styleeditor.xul, scratchpad.js and scratchpad.xul. Thank you!
Keywords: dev-doc-needed
Created attachment 596046 [details] [diff] [review]
Rebase
Attachment #595816 - Attachment is obsolete: true
Attachment #595816 - Flags: review?(rcampbell)
Attachment #596046 - Flags: review?(rcampbell)
(Assignee)

Comment 11

5 years ago
Comment on attachment 595816 [details] [diff] [review]
rebased patch

This is the correct patch for review.
Attachment #595816 - Attachment is obsolete: false
Attachment #595816 - Flags: review?(rcampbell)
(Assignee)

Comment 12

5 years ago
Comment on attachment 596046 [details] [diff] [review]
Rebase

This patch is missing a fix in source-editor.jsm.
Attachment #596046 - Attachment is obsolete: true
Attachment #596046 - Flags: review?(rcampbell)
Depends on: 726074
Blocks: 727450
(Assignee)

Comment 13

5 years ago
Created attachment 597430 [details] [diff] [review]
dtd fix

Added the new sourceeditor.dtd to the browser/locales/jar.mn file. Somehow this chunk of code got lost in the patch rebases and Firefox didn't throw any errors. Big thanks to Mike for his help with reporting and identifying the problem!
Attachment #595816 - Attachment is obsolete: true
Attachment #595816 - Flags: review?(rcampbell)
Attachment #597430 - Flags: review?(rcampbell)
(Assignee)

Comment 14

5 years ago
Do we want to add Find, find again and jump to line in the default context menu? The current context menu I implemented here has the same options as the default textarea context menu. I would say it makes sense for a source editor to have more options, but that argument is also a slippery slope to add too many items - I'd like us to be picky on what we add.
(Assignee)

Comment 15

5 years ago
Try server result are very much green:

https://tbpl.mozilla.org/?tree=Try&rev=0cc7f7d26343

(pushed this patch and all the dependencies)
Comment on attachment 597430 [details] [diff] [review]
dtd fix

in source-editor-overlay.xul:

+
+  <!-- This Source Editor overlay requires that the editMenuOverlay.xul is
+       loaded, and you also need the globalOverlay.js script in the XUL document
+       where the source-editor-overlay.xul is loaded. -->

This comment is a little confusing. I think some slight rewording makes it clearer:

+  <!-- This Source Editor overlay requires the editMenuOverlay.xul to
+       be loaded. The globalOverlay.js script is also required in the
+       XUL document where the source-editor-overlay.xul is loaded. -->

+  _onUndoRedo: function SEU__onUndoRedo()
+  {
+    if (this._ownerWindow.goUpdateCommand) {
+      this._ownerWindow.goUpdateCommand("se-cmd-undo");
+      this._ownerWindow.goUpdateCommand("se-cmd-redo");
+    }

not really sure this needs to be in a separate method.

could just put that into _onDirtyChanged directly.

no tests with this. I believe we talked about this being somewhat difficult to write a test that actually works properly due to widget issues.
Attachment #597430 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 17

5 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #16)
> Comment on attachment 597430 [details] [diff] [review]
> dtd fix
> 
> in source-editor-overlay.xul:
> 
> +
> +  <!-- This Source Editor overlay requires that the editMenuOverlay.xul is
> +       loaded, and you also need the globalOverlay.js script in the XUL
> document
> +       where the source-editor-overlay.xul is loaded. -->
> 
> This comment is a little confusing. I think some slight rewording makes it
> clearer:
> 
> +  <!-- This Source Editor overlay requires the editMenuOverlay.xul to
> +       be loaded. The globalOverlay.js script is also required in the
> +       XUL document where the source-editor-overlay.xul is loaded. -->

Thanks! My English!... :)


> +  _onUndoRedo: function SEU__onUndoRedo()
> +  {
> +    if (this._ownerWindow.goUpdateCommand) {
> +      this._ownerWindow.goUpdateCommand("se-cmd-undo");
> +      this._ownerWindow.goUpdateCommand("se-cmd-redo");
> +    }
> 
> not really sure this needs to be in a separate method.
> 
> could just put that into _onDirtyChanged directly.

I thought I can do that, but there are still cases where undo/redo need to be tracked, and the dirty change state event is not enough.
(Assignee)

Comment 18

5 years ago
Created attachment 598513 [details] [diff] [review]
comment fix

Thanks Rob for the r+!

The last patch needing a review in this patch queue is the one in bug 700893.
Attachment #597430 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 599759 [details] [diff] [review]
added more menu items

Updated the patch to include find, find again and jump to line in the default context menu.
Attachment #598513 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 599760 [details] [diff] [review]
interdiff

Interdiff to make it easier for you to glance over the latest changes. Take a quick look and let me know if it's fine.

One note: it would be nice if we'd have less keys defined in scratchpad.xul and more editMenuKeys reused - like I did in the style editor code. I could remove more keys from scratchpad and replace them with the editMenuKeys.
Attachment #599760 - Flags: feedback?(rcampbell)
(Assignee)

Comment 21

5 years ago
Green results on try:

https://tbpl.mozilla.org/?tree=Try&rev=ff8bf938434a
(Assignee)

Updated

5 years ago
Blocks: 730061
Comment on attachment 599760 [details] [diff] [review]
interdiff

extra indentation in sourceeditor.dtd.

f+
Attachment #599760 - Flags: feedback?(rcampbell) → feedback+
(In reply to Mihai Sucan [:msucan] from comment #20)
> Created attachment 599760 [details] [diff] [review]
> interdiff
> 
> Interdiff to make it easier for you to glance over the latest changes. Take
> a quick look and let me know if it's fine.
> 
> One note: it would be nice if we'd have less keys defined in scratchpad.xul
> and more editMenuKeys reused - like I did in the style editor code. I could
> remove more keys from scratchpad and replace them with the editMenuKeys.

yeah, agreed. We should file a follow-up bug to do that.
(Assignee)

Comment 24

5 years ago
Created attachment 600981 [details] [diff] [review]
[in-fx-team] indentation fix

Thanks Rob for the f+!

Landed this patch in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/0c37652c28ae
Attachment #599759 - Attachment is obsolete: true
Attachment #599760 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
(Assignee)

Comment 25

5 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #23)
> (In reply to Mihai Sucan [:msucan] from comment #20)
> > One note: it would be nice if we'd have less keys defined in scratchpad.xul
> > and more editMenuKeys reused - like I did in the style editor code. I could
> > remove more keys from scratchpad and replace them with the editMenuKeys.
> 
> yeah, agreed. We should file a follow-up bug to do that.

Filed bug 730898.
https://hg.mozilla.org/mozilla-central/rev/0c37652c28ae
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [sourceeditor][fixed-in-fx-team] → [sourceeditor]
Target Milestone: --- → Firefox 13
Duplicate of this bug: 715306
You need to log in before you can comment on or make changes to this bug.