Summary: | Web Inspector: Pressing tab in the styles sidebar shouldn't insert a tab character | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | 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
Devin Rousso
2015-06-20 22:19:32 PDT
Created attachment 255313 [details]
Patch
Created attachment 255328 [details]
Patch
Can you post a video of this in action? 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.
Created attachment 255467 [details]
Patch
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()? Created attachment 255517 [details]
Patch
Comment on attachment 255517 [details] Patch Clearing flags on attachment: 255517 Committed r185935: <http://trac.webkit.org/changeset/185935> All reviewed patches have been landed. Closing bug. |