WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(11.04 KB, patch)
2010-12-09 02:19 PST
,
Andrey Adaikin
no flags
Details
Formatted Diff
Diff
Patch
(11.04 KB, patch)
2010-12-09 04:35 PST
,
Andrey Adaikin
no flags
Details
Formatted Diff
Diff
Patch
(42.82 KB, patch)
2011-01-25 05:40 PST
,
Andrey Adaikin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Andrey Adaikin
Comment 1
2010-12-06 01:22:34 PST
Created
attachment 75657
[details]
Patch
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
Created
attachment 76029
[details]
Patch
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
Created
attachment 76046
[details]
Patch
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
Created
attachment 80053
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug