Bug 146189

Summary: Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=145885
Attachments:
Description Flags
Patch
none
Patch
none
Tab Functionality (after patch is applied)
none
Patch
none
Patch none

Description Devin Rousso 2015-06-20 22:19:32 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-06-20 22:19:44 PDT
<rdar://problem/21476238>
Comment 2 Devin Rousso 2015-06-20 22:30:16 PDT
Created attachment 255313 [details]
Patch
Comment 3 Devin Rousso 2015-06-21 10:42:00 PDT
Created attachment 255328 [details]
Patch
Comment 4 Timothy Hatcher 2015-06-21 19:37:09 PDT
Can you post a video of this in action?
Comment 5 Devin Rousso 2015-06-22 11:05:57 PDT
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.
Comment 6 Devin Rousso 2015-06-23 19:01:06 PDT
Created attachment 255467 [details]
Patch
Comment 7 Timothy Hatcher 2015-06-24 14:24:56 PDT
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()?
Comment 8 Devin Rousso 2015-06-24 15:14:32 PDT
Created attachment 255517 [details]
Patch
Comment 9 WebKit Commit Bot 2015-06-24 16:55:02 PDT
Comment on attachment 255517 [details]
Patch

Clearing flags on attachment: 255517

Committed r185935: <http://trac.webkit.org/changeset/185935>
Comment 10 WebKit Commit Bot 2015-06-24 16:55:05 PDT
All reviewed patches have been landed.  Closing bug.