731394 - Source Editor in read only mode is not entirely read only

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

As reported by Panos in bug 723069 comment 2: users can still make changes to the text when the editor is in read only mode: Enter, Tab and Shift-Tab make changes to the code. Beyond that, the context menus in the Style Editor and Scratchpad show Cut, Paste and Delete as enabled items which would work. But when you try to Cut and Paste the actions do not work - as expected in read only mode. Same goes for the Edit > Cut and Paste menu items from Scratchpad. The Delete context menu item semi-works: it deletes the text from the Orion contentEditable, but Orion itself goes bonkers - the native cmd_delete badly breaks Orion in read only mode.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: wip

Submitting a WIP patch with the complete fix for all the problems mentioned in comment 0.

Not taking the bug because I'm trying to take a break from Source Editor work, but given the high priority of this bug, I wanted to have a patch. Will see about writing a test - or if someone else wants to do that, I'd really appreciate that! Thanks!

I already spent more time than needed on the patch: it seems that the reason the Source Editor nsIController can't override the native editor nsIController commands is because the native editor nsIController is often readded to the list of controllers at index 0 (the first controller). Orion keeps changing the contentEditable state of its elements - causing native code to add/remove the native editor nsIController. Therefore commands like cmd_undo, cmd_redo, cmd_cut, cmd_paste, cmd_delete need to be implemented by the Source Editor controller with different names. Mini-forking of code paths. Yay! Fun!

Comment 2

[Mihai Sucan [:msucan]]

This patch also adds the default Source Editor context menu to the JS debugger, which means it will fix bug 730061.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 601399 [details] [diff] [review]
wip

+    dbg.editor.resetUndo();

will this addition prevent the undo key from reverting while in read-only mode when the contents change? (this currently happens in the debugger).

Yeah, this should do it.

Comment 4

[Mihai Sucan [:msucan]]

Thanks for the f+!


(In reply to Rob Campbell [:rc] (robcee) from comment #3)
> Comment on attachment 601399 [details] [diff] [review]
> wip
> 
> +    dbg.editor.resetUndo();
> 
> will this addition prevent the undo key from reverting while in read-only
> mode when the contents change? (this currently happens in the debugger).

Yes. The way Orion works with read-only is that it's read-only only to the user. It always allows changes from code. So, if the code does setText() then we allow undo(). For that reason I call resetUndo() to clear the history of changes.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch which includes test code as well.

Notes: as discussed on IRC here we are in a bit of "ugly mess" situation. I tried my best to make things as clear as possible.

What I did: since we are required to make our own cmd_undo/redo/cut/paste/etc commands, we were required to make our own key_undo/redo/cut/paste/etc keys as well. We can't overwrite the commands associated in the key elements coming from the editMenuKeys. This overall had the impact of copying a lot of stuff from the edit menu overlay.

Now the overlay provides easy-to-reuse menuitems like the editMenuOverlay. Now scratchpad no longer needs to have its own edit menuitems, keys or commands. We can just reference the menuitems, keys and commands from the source editor overlay. The same goes for the debugger and style editor. This allowed me to remove always-disabled menuitems from the debugger context menu (eg. I removed undo, cut, paste, delete).

There are no more duplicates in scratchpad - we no longer redefine keys, menuitems or commands in scratchpad.

The only remaining problem (which we can't yet fix) is the duplication of commands, keys and menuitems between the source editor overlay and the edit menu overlay. This is something we can't solve (for now), because we needed to avoid using the native commands as explained in comment 1. Nonetheless, I have reused the editMenuOverlay.dtd in the source editor overlay.

Keyboard shortcuts are now the same in the debugger, style editor, scratchpad and in other places where the edit menu overlay is used. (I did use the same shortcuts in the source editor overlay as in the edit menu overlay.)

The diff is uglier than the result. Please let me know if you have any concerns. (I know I felt like writing a "heretic" patch :) ) Comments are very much welcome!

Thank you!

Comment 6

[Mihai Sucan [:msucan]]

This patch depends on the work done in bug 730898.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 603817 [details] [diff] [review]
proposed patch

in debugger.js:

_showScript:

+      window.editor.resetUndo();

can be moved outside the if... else.

oof. this is complicated, but I don't have a better suggestion for how to make this cleaner. It does simplify the dtds at least.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] updated patch

Thanks for the r+! Updated _showScript() as suggested. I missed that while working on it.

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 604098 [details] [diff] [review]
[in-fx-team] updated patch

Landed:
https://hg.mozilla.org/integration/fx-team/rev/310226a5234a

Thanks for the review!

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/310226a5234a