Bug 146189 – Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

In the styles sidebar, tab's are not very useful and don't have many use cases.  Instead, pressing tab should move the cursor from property name to property value and, once the end of the line is reached, on to the next line.  This should not interfere with the autocorrecting functionality but should take effect in most other situations.

[Radar WebKit Bug Importer]

<rdar://problem/21476238>

[Devin Rousso]

Created attachment 255313 [details]
Patch

[Devin Rousso]

Created attachment 255328 [details]
Patch

[Timothy Hatcher]

Can you post a video of this in action?

[Devin Rousso]

Created attachment 255355 [details]
Tab Functionality (after patch is applied)

Each time the cursor moves forward, I am pressing the tab key.  When it moves backwards, I am pressing shift-tab.

[Devin Rousso]

Created attachment 255467 [details]
Patch

[Timothy Hatcher]

Comment on attachment 255467 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:42
> +        this._noCSSEndingSemicolon = false;

I don't think you need CSS here. It could be used by JS too.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:342
> +        if (reverse && typeof this._delegate.cssStyleDeclarationSectionEditorPrevRule === "function")

Nit: Spell out Previous.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:472
> +        if (event.keyCode === 9) {

Does event.keyIdentifier === "Tab" work? This should also be an early return to avoid indenting the whole function body.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:69
> +        this._completionController._noCSSEndingSemicolon = true;

This uses a private property from another file. A no-no for us. It should be exposed as a public property with no underscore if it needed used externally.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:350
> +        if (!line || !line.trim().length)

Put the trimmed string in a local var to avoid doing it twice.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:354
> +        this._codeMirror.setSelection({line: 0, ch: 0}, {line: 0, ch: index < 0 ? line.trim().length : index});

trim() will remove prefix space, so this could put the end of the selection at the wrong column. You should use trimRight().

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:363
> +        this._codeMirror.setSelection({line, ch: colon ? colon.index + colon[0].length : 0}, {line, ch: lineText.trim().length - lineText.trim().endsWith(";")});

You should put lineText.trim() in a local var to avoid doing it twice. Also, trimRight().

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:395
> +        cursor.ch = line.trim().length;

Put the trimmed in a local. trimRight()?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:419
> +        var prevLine = codeMirror.getLine(cursor.line - 1);

Nit: previousLine

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:470
> +        var trimmedLength = line.trim().length;

Put the trimmed in a local. trimRight()?

[Devin Rousso]

Created attachment 255517 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 255517 [details]
Patch

Clearing flags on attachment: 255517

Committed r185935: <http://trac.webkit.org/changeset/185935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.