Bug 50553 – Web Inspector: make script view editable

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

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.

[Andrey Adaikin]

Created attachment 75657 [details]
Patch

[Pavel Feldman]

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 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]

> >> 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]

Created attachment 76029 [details]
Patch

[Alexander Pavlov (apavlov)]

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 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]

Created attachment 76046 [details]
Patch

[Pavel Feldman]

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]

Created attachment 80053 [details]
Patch

[Andrey Adaikin]

*** This bug has been marked as a duplicate of bug 53588 ***

[Pavel Feldman]

Comment on attachment 80053 [details]
Patch

Clearing r? from the obsolete patch.