RESOLVED DUPLICATE of bug 53588 50553
Web Inspector: make script view editable
https://bugs.webkit.org/show_bug.cgi?id=50553
Summary Web Inspector: make script view editable
Yury Semikhatsky
Reported 2010-12-06 00:47:20 PST
Web Inspector: make Script view editable. Now it's possible to make changes in live-edit mode using double-click to start editing. Would be better to have its content editable like in classic IDEs.
Attachments
Patch (44.08 KB, patch)
2010-12-06 01:22 PST, Andrey Adaikin
no flags
Patch (11.04 KB, patch)
2010-12-09 02:19 PST, Andrey Adaikin
no flags
Patch (11.04 KB, patch)
2010-12-09 04:35 PST, Andrey Adaikin
no flags
Patch (42.82 KB, patch)
2011-01-25 05:40 PST, Andrey Adaikin
no flags
Andrey Adaikin
Comment 1 2010-12-06 01:22:34 PST
Pavel Feldman
Comment 2 2010-12-07 06:25:00 PST
Comment on attachment 75657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75657&action=review > WebCore/inspector/front-end/SourceTokenizer.js:-53 > - return this._condition.subTokenizer; This is likely to break HTML tokenizer (it has JavaScript and CSS subtokenizers). > WebCore/inspector/front-end/TextEditorHighlighter.js:46 > + this._tokenizerCondition = JSON.stringify(this._tokenizer.initialCondition); What was wrong with the object state? > WebCore/inspector/front-end/TextEditorHighlighter.js:54 > + this._tokenizerCondition = JSON.stringify(this._tokenizer.initialCondition); ditto > WebCore/inspector/front-end/TextEditorHighlighter.js:141 > + return; run WebKitTools/Scripts/check-webkit-style to make sure there are no trailing spaces. > WebCore/inspector/front-end/TextEditorModel.js:37 > + this.upwards = upwards; startLine can be > than endLine, there is no need for "upwards" flag. > WebCore/inspector/front-end/TextViewer.js:36 > + this._textModel.resetUndoStack(); I think undo/redo support can be added in a separate change. > WebCore/inspector/front-end/TextViewer.js:54 > + this.element.addEventListener("dragstart", this._preventDefaultAndStopPropagation.bind(this), false); Drag'n'drop should not be disabled. > WebCore/inspector/front-end/TextViewer.js:58 > + // TODO(aandrey): Disable the "Delete" context menu. HOW TO?! We use // FIXME: foo bar. style in WebKit. > WebCore/inspector/front-end/TextViewer.js:174 > + } else { no need for { } in for single line blocks. > WebCore/inspector/front-end/TextViewer.js:254 > + } else { ditto > WebCore/inspector/front-end/TextViewer.js:263 > + for (var i = 0; i < removedChunks.length; ++i) { ditto > WebCore/inspector/front-end/TextViewer.js:411 > + this._replaceSelectionWith("\t"); This is very fishy. You should not care about the nature of the change (Tab vs Enter vs Backspace vs whatever). You should be able to re-build text model based on the generic dom change. > WebCore/inspector/front-end/TextViewer.js:438 > + this._shortcuts[WebInspector.KeyboardShortcut.makeKey("z", modifiers.CtrlOrMeta)] = this._handleUndo.bind(this); All of these are supported by the contentEditable. You should not register them.
Andrey Adaikin
Comment 3 2010-12-08 02:33:20 PST
Comment on attachment 75657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75657&action=review Thanks for the review. I will discard this change and start sending smaller ones to preserve existing live editing functionality. >> WebCore/inspector/front-end/SourceTokenizer.js:-53 >> - return this._condition.subTokenizer; > > This is likely to break HTML tokenizer (it has JavaScript and CSS subtokenizers). I searched carefully for the "subTokenizer" string and did not find it anywhere. But I can leave this as is though :) >> WebCore/inspector/front-end/TextEditorHighlighter.js:46 >> + this._tokenizerCondition = JSON.stringify(this._tokenizer.initialCondition); > > What was wrong with the object state? It worked in your case because "initialCondition" and "condition" was a number, but now they are objects. This JSON stringify/parse solution will work for both cases. >> WebCore/inspector/front-end/TextEditorHighlighter.js:141 >> + return; > > run WebKitTools/Scripts/check-webkit-style to make sure there are no trailing spaces. Thanks. >> WebCore/inspector/front-end/TextEditorModel.js:37 >> + this.upwards = upwards; > > startLine can be > than endLine, there is no need for "upwards" flag. The implementation of the TextEditorModel class (implicitly) assumes that {startPoint} <= {endPoint}. I was thinking that this is an invariant to be held. If I remove this flag and break the invariant, I'll have to modify a whole bunch of code to respect this. What is the preferred way according to you? >> WebCore/inspector/front-end/TextViewer.js:36 >> + this._textModel.resetUndoStack(); > > I think undo/redo support can be added in a separate change. Yes. I'm going to discard this large change and send several incremental (hopefully small) changes. >> WebCore/inspector/front-end/TextViewer.js:54 >> + this.element.addEventListener("dragstart", this._preventDefaultAndStopPropagation.bind(this), false); > > Drag'n'drop should not be disabled. I also think so. Added FIXME for the future, but will leave this as is for now because default implementation breaks the editor. >> WebCore/inspector/front-end/TextViewer.js:58 >> + // TODO(aandrey): Disable the "Delete" context menu. HOW TO?! > > We use // FIXME: foo bar. style in WebKit. OK. >> WebCore/inspector/front-end/TextViewer.js:174 >> + } else { > > no need for { } in for single line blocks. Done. >> WebCore/inspector/front-end/TextViewer.js:254 >> + } else { > > ditto Done. >> WebCore/inspector/front-end/TextViewer.js:263 >> + for (var i = 0; i < removedChunks.length; ++i) { > > ditto Done. Also found all places like this. >> WebCore/inspector/front-end/TextViewer.js:411 >> + this._replaceSelectionWith("\t"); > > This is very fishy. You should not care about the nature of the change (Tab vs Enter vs Backspace vs whatever). You should be able to re-build text model based on the generic dom change. As far as I understand you, and extending your logic further, I might say that "You should not care about keypress,keydown,copy,paste,cut and other events. You should be able to re-build text model based on generic dom change." If my understanding is correct, then I agree with you - we are much better off listening to a single "DOM changed" event that is fired when a content editable DOM changes. But AFAIK there is no such event :) Please correct me if I'm wrong. >> WebCore/inspector/front-end/TextViewer.js:438 >> + this._shortcuts[WebInspector.KeyboardShortcut.makeKey("z", modifiers.CtrlOrMeta)] = this._handleUndo.bind(this); > > All of these are supported by the contentEditable. You should not register them. I will investigate this.
Pavel Feldman
Comment 4 2010-12-08 04:08:52 PST
> >> WebCore/inspector/front-end/SourceTokenizer.js:-53 > >> - return this._condition.subTokenizer; > > > > This is likely to break HTML tokenizer (it has JavaScript and CSS subtokenizers). > > I searched carefully for the "subTokenizer" string and did not find it anywhere. But I can leave this as is though :) > You are right, it is not used now since subtokenizers got hardcoded in HTML tokenizer once we migrated from TextEditor to TextViewer earlier. > >> WebCore/inspector/front-end/TextEditorHighlighter.js:46 > >> + this._tokenizerCondition = JSON.stringify(this._tokenizer.initialCondition); > > > > What was wrong with the object state? > > It worked in your case because "initialCondition" and "condition" was a number, but now they are objects. This JSON stringify/parse solution will work for both cases. > Why can't we store a reference to an object without the need to stringify it? > >> WebCore/inspector/front-end/TextEditorModel.js:37 > >> + this.upwards = upwards; > > > > startLine can be > than endLine, there is no need for "upwards" flag. > > The implementation of the TextEditorModel class (implicitly) assumes that {startPoint} <= {endPoint}. I was thinking that this is an invariant to be held. If I remove this flag and break the invariant, I'll have to modify a whole bunch of code to respect this. What is the preferred way according to you? > Again, before we migrated to the TextViewer, we had WebInspector.TextSelectionModel. It was directional, while range was just range. TextModel was operating TextRanges, while editor was operating Selections (check out http://repenaxa.com/editor/editor.html). If you need a selection representation, I'd suggest that you use a separate abstraction for it. > >> WebCore/inspector/front-end/TextViewer.js:411 > >> + this._replaceSelectionWith("\t"); > > > > This is very fishy. You should not care about the nature of the change (Tab vs Enter vs Backspace vs whatever). You should be able to re-build text model based on the generic dom change. > > As far as I understand you, and extending your logic further, I might say that "You should not care about keypress,keydown,copy,paste,cut and other events. You should be able to re-build text model based on generic dom change." If my understanding is correct, then I agree with you - we are much better off listening to a single "DOM changed" event that is fired when a content editable DOM changes. But AFAIK there is no such event :) Please correct me if I'm wrong. Yes, this is exactly what I mean - we should listen to DOM. Check out DOMCharacterDataModified event for that: Try document.addEventListener("DOMCharacterDataModified", function(e) { console.log(e); }, false);
Andrey Adaikin
Comment 5 2010-12-09 02:19:56 PST
Alexander Pavlov (apavlov)
Comment 6 2010-12-09 02:31:04 PST
Comment on attachment 76029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76029&action=review > WebCore/inspector/front-end/TextViewer.js:349 > + _beginDomUpdates: function() In WebKit, all acronyms use upper case (HTML, XML, DOM, CSS, etc.) - see http://webkit.org/coding/coding-style.html for reference > WebCore/inspector/front-end/TextViewer.js:351 > + this._modifyingDomLevel++; ditto > WebCore/inspector/front-end/TextViewer.js:354 > + _endDomUpdates: function() ditto > WebCore/inspector/front-end/TextViewer.js:356 > + this._modifyingDomLevel--; ditto
Andrey Adaikin
Comment 7 2010-12-09 04:33:34 PST
Comment on attachment 76029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76029&action=review >> WebCore/inspector/front-end/TextViewer.js:349 >> + _beginDomUpdates: function() > > In WebKit, all acronyms use upper case (HTML, XML, DOM, CSS, etc.) - see http://webkit.org/coding/coding-style.html for reference Done in all places.
Andrey Adaikin
Comment 8 2010-12-09 04:35:01 PST
Pavel Feldman
Comment 9 2010-12-09 07:17:58 PST
Comment on attachment 76046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76046&action=review Few nits to fix and it is good to land. > WebCore/inspector/front-end/TextViewer.js:58 > + this._linesContainerElement.addEventListener("DOMCharacterDataModified", this._handleDOMCharacterDataModified.bind(this), false); You should only add those in case of editable mode. > WebCore/inspector/front-end/TextViewer.js:94 > + this._linesContainerElement.setAttribute("contentEditable", !!this._editCallback); this._linesContainerElement.contentEditable = !!this._editCallback; > WebCore/inspector/front-end/TextViewer.js:241 > _handleKeyDown: function() I think you should conditionally add listeners instead. > WebCore/inspector/front-end/TextViewer.js:272 > _handleDoubleClick: function(e) ditto
Andrey Adaikin
Comment 10 2011-01-25 05:40:54 PST
Andrey Adaikin
Comment 11 2011-02-02 06:28:56 PST
*** This bug has been marked as a duplicate of bug 53588 ***
Pavel Feldman
Comment 12 2011-02-25 03:02:48 PST
Comment on attachment 80053 [details] Patch Clearing r? from the obsolete patch.
Note You need to log in before you can comment on or make changes to this bug.