Web Inspector: CodeMirror still inserts a tab even when "Prefer indent using" is set to "Spaces"

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. go to any resource that is editable
3. press tab ↹
 => \t inserted instead of "    " (4)

[Devin Rousso]

Created attachment 376384 [details]
Patch

[Ross Kirsling]

Is this better than having WI make Tab execute `insertSoftTab` and specify `options.tabSize` appropriately?

[Devin Rousso]

(In reply to Ross Kirsling from comment #2)
> Is this better than having WI make Tab execute `insertSoftTab` and specify `options.tabSize` appropriately?
I didn't even know that was a thing!  I'll give it a shot =D

[Devin Rousso]

Created attachment 376387 [details]
Patch

[Ross Kirsling]

Comment on attachment 376387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376387&action=review

It's still a bit sad to me, but I guess it's how we've got to do it:
https://github.com/codemirror/CodeMirror/pull/4317

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:624
> +        CodeMirror.commands.insertTab = function(cm) {

Maybe it'd still be better to overwrite `defaultTab`, so that you don't have to store `original`?

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:625
> +            if (WI.settings.indentWithTabs.value)

I think this can be `cm.options.indentWithTabs`, given all this:
https://github.com/WebKit/webkit/blob/master/Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js#L36-L58

[Devin Rousso]

Comment on attachment 376387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376387&action=review

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:624
>> +        CodeMirror.commands.insertTab = function(cm) {
> 
> Maybe it'd still be better to overwrite `defaultTab`, so that you don't have to store `original`?

I'd rather not, as that would require that I duplicate the `if (cm.somethingSelected()) { cm.indentSelection("add") }` logic, which adds complexity and potential confusion each time we update CodeMirror.

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:625
>> +            if (WI.settings.indentWithTabs.value)
> 
> I think this can be `cm.options.indentWithTabs`, given all this:
> https://github.com/WebKit/webkit/blob/master/Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js#L36-L58

Good point.  I'll switch.

[Devin Rousso]

Created attachment 376390 [details]
Patch

[Ross Kirsling]

(In reply to Devin Rousso from comment #6)
> I'd rather not, as that would require that I duplicate the `if
> (cm.somethingSelected()) { cm.indentSelection("add") }` logic, which adds
> complexity and potential confusion each time we update CodeMirror.

Even with this solution though, I wonder if a comment would be in order to encourage us to keep an eye on it across CodeMirror updates? (Maybe we could link to the PR as a justification for why we had to go this route.)

[Ross Kirsling]

Comment on attachment 376390 [details]
Patch

r=me otherwise

[Devin Rousso]

Created attachment 376396 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376396 [details]
Patch

Clearing flags on attachment: 376396

Committed r248739: <https://trac.webkit.org/changeset/248739>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54358187>