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.
Created attachment 75657 [details] Patch
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.
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.
> >> 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);
Created attachment 76029 [details] Patch
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
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.
Created attachment 76046 [details] Patch
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
Created attachment 80053 [details] Patch
*** This bug has been marked as a duplicate of bug 53588 ***
Comment on attachment 80053 [details] Patch Clearing r? from the obsolete patch.