Changeset 184000 in webkit


Ignore:
Timestamp:
May 8, 2015 11:37:49 AM (9 years ago)
Author:
commit-queue@webkit.org
Message:

Web Inspector: Styles sidebar editing with incomplete property looks poor in UI
​https://bugs.webkit.org/show_bug.cgi?id=141692

Patch by Tobias Reiss <​tobi+webkit@basecode.de> on 2015-05-08
Reviewed by Timothy Hatcher.

Add "css-rule" Formatter that breaks CSS declarations into multiple lines,
keeps comments and invalid styles and adds whitespace.

  • Tools/PrettyPrinting/css-rule-tests/*.css: Added.

Add test cases.

  • Tools/PrettyPrinting/index.html:

Enable Test setup to be able to run "css-rule" Formatter tests.

  • UserInterface/Controllers/Formatter.js:

(Formatter.prototype._handleToken):

  • UserInterface/Controllers/FormatterContentBuilder.js:

(FormatterContentBuilder.prototype.removeLastNewline):
(FormatterContentBuilder.prototype.removeLastWhitespace):
(FormatterContentBuilder.prototype._popFormattedContent):
(FormatterContentBuilder.prototype._popNewLine): Deleted.

  • UserInterface/Views/CSSStyleDeclarationTextEditor.js:

(WebInspector.CSSStyleDeclarationTextEditor.prototype._formattedContentFromEditor):
(WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.set this):
(WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.get this):
(WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update):
(WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent):
(WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.countNewLineCharacters): Deleted.
(WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.else): Deleted.

  • UserInterface/Views/CodeMirrorFormatters.js:
Location:
trunk/Source/WebInspectorUI
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebInspectorUI/ChangeLog

    r183947 r184000  
     12015-05-08  Tobias Reiss  <tobi+webkit@basecode.de>
     2
     3        Web Inspector: Styles sidebar editing with incomplete property looks poor in UI
     4        https://bugs.webkit.org/show_bug.cgi?id=141692
     5
     6        Reviewed by Timothy Hatcher.
     7
     8        Add "css-rule" Formatter that breaks CSS declarations into multiple lines,
     9        keeps comments and invalid styles and adds whitespace.
     10
     11        * Tools/PrettyPrinting/css-rule-tests/*.css: Added.
     12        Add test cases.
     13
     14        * Tools/PrettyPrinting/index.html:
     15        Enable Test setup to be able to run "css-rule" Formatter tests.
     16
     17        * UserInterface/Controllers/Formatter.js:
     18        (Formatter.prototype._handleToken):
     19        * UserInterface/Controllers/FormatterContentBuilder.js:
     20        (FormatterContentBuilder.prototype.removeLastNewline):
     21        (FormatterContentBuilder.prototype.removeLastWhitespace):
     22        (FormatterContentBuilder.prototype._popFormattedContent):
     23        (FormatterContentBuilder.prototype._popNewLine): Deleted.
     24        * UserInterface/Views/CSSStyleDeclarationTextEditor.js:
     25        (WebInspector.CSSStyleDeclarationTextEditor.prototype._formattedContentFromEditor):
     26        (WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.set this):
     27        (WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.get this):
     28        (WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update):
     29        (WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent):
     30        (WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.countNewLineCharacters): Deleted.
     31        (WebInspector.CSSStyleDeclarationTextEditor.prototype._resetContent.update.else): Deleted.
     32        * UserInterface/Views/CodeMirrorFormatters.js:
     33
    1342015-05-07  Joseph Pecoraro  <pecoraro@apple.com>
    235

  • trunk/Source/WebInspectorUI/Tools/PrettyPrinting/index.html

    r183043 r184000  
    99    <script src="../../UserInterface/External/CodeMirror/javascript.js"></script>
    1010    <script src="../../UserInterface/External/CodeMirror/css.js"></script>
     11    <script src="../../UserInterface/Base/WebInspector.js"></script>
     12    <script src="../../UserInterface/Views/CodeMirrorAdditions.js"></script>
    1113    <script src="../../UserInterface/Controllers/Formatter.js"></script>
    1214    <script src="FormatterDebug.js"></script>
    … …  
    2224        <option selected value="text/javascript">JavaScript</option>
    2325        <option value="text/css">CSS</option>
     26        <option value="css-rule">CSS-Rule</option>
    2427    </select>
    2528    <button id="populate">Populate</button>
    … …  
    231234        if (modePicker.value === "text/javascript")
    232235            runJavaScriptTests(completedCallback);
     236        else if (modePicker.value === "css-rule")
     237            runCssRuleTests(completedCallback);
    233238        else
    234239            runCSSTests(completedCallback);
    … …  
    239244            "js-tests/single-statement-blocks.js",
    240245            "js-tests/switch-case-default.js",
     246        ]);
     247    }
     248    function runCssRuleTests(callback) {
     249        _runTests(callback, [
     250            "css-rule-tests/invalid-property-is-not-removed.css",
     251            "css-rule-tests/remove-whitespace-before-colon.css",
     252            "css-rule-tests/remove-whitespace-before-semicolon.css",
     253            "css-rule-tests/remove-whitespace-before-property.css",
     254            "css-rule-tests/remove-whitespace-before-prefixed-property.css",
     255            "css-rule-tests/remove-whitespace-before-invalid-property.css",
     256            "css-rule-tests/remove-whitespace-before-comment.css",
     257            "css-rule-tests/split-comment-followed-by-property.css",
     258            "css-rule-tests/split-comment-followed-by-prefixed-property.css",
     259            "css-rule-tests/split-comment-followed-by-invalid-property.css",
     260            "css-rule-tests/split-comment-followed-by-comment.css",
     261            "css-rule-tests/split-property-followed-by-property.css",
     262            "css-rule-tests/split-property-followed-by-prefixed-property.css",
     263            "css-rule-tests/split-property-followed-by-invalid-property.css",
     264            "css-rule-tests/split-property-followed-by-comment.css",
     265            "css-rule-tests/split-invalid-property-followed-by-property.css",
     266            "css-rule-tests/split-invalid-property-followed-by-prefixed-property.css",
     267            "css-rule-tests/split-invalid-property-followed-by-invalid-property.css",
     268            "css-rule-tests/split-invalid-property-followed-by-comment.css",
     269            "css-rule-tests/split-property-without-semicolon-followed-by-comment-and-property.css",
     270            "css-rule-tests/add-whitespace-after-colon.css",
     271            "css-rule-tests/add-whitespace-after-comma.css",
     272            "css-rule-tests/do-not-append-semicolon.css",
     273            "css-rule-tests/keep-prefixed-value.css",
    241274        ]);
    242275    }

  • trunk/Source/WebInspectorUI/UserInterface/Controllers/Formatter.js

    r181844 r184000  
    9090            this._builder.appendNewline();
    9191
    92         // Whitespace. Collapse to a single space.
     92        // Whitespace. Remove all spaces or collapse to a single space.
    9393        if (isWhiteSpace) {
    9494            this._builder.appendSpace();
    … …  
    101101        if (mode.modifyStateForTokenPre)
    102102            mode.modifyStateForTokenPre(this._lastToken, this._lastContent, token, state, content, isComment);
     103
     104        // Should we remove the last whitespace?
     105        if (this._builder.lastTokenWasWhitespace && mode.removeLastWhitespace(this._lastToken, this._lastContent, token, state, content, isComment))
     106            this._builder.removeLastWhitespace();
    103107
    104108        // Should we remove the last newline?

  • trunk/Source/WebInspectorUI/UserInterface/Controllers/FormatterContentBuilder.js

    r181844 r184000  
    11/*
    22 * Copyright (C) 2013 Apple Inc. All rights reserved.
     3 * Copyright (C) 2015 Tobias Reiss <tobi+webkit@basecode.de>
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    138139        console.assert(this._formattedContent.lastValue === "\n");
    139140        if (this.lastTokenWasNewline) {
    140             this._popNewLine();
     141            this._popFormattedContent();
     142            this._formattedLineEndings.pop();
    141143            this._startOfLine = false;
    142144            this.lastTokenWasNewline = false;
    … …  
    145147    }
    146148
     149    removeLastWhitespace()
     150    {
     151        console.assert(this.lastTokenWasWhitespace);
     152        console.assert(this._formattedContent.lastValue === " ");
     153        if (this.lastTokenWasWhitespace) {
     154            this._popFormattedContent();
     155            // No need to worry about `_startOfLine` and `lastTokenWasNewline`
     156            // because `appendSpace` takes care of not adding whitespace
     157            // to the beginning of a line.
     158            this.lastTokenWasWhitespace = false;
     159        }
     160    }
     161
    147162    indent()
    148163    {
    … …  
    171186    // Private
    172187
    173     _popNewLine()
     188    _popFormattedContent()
    174189    {
    175190        var removed = this._formattedContent.pop();
    176191        this._formattedContentLength -= removed.length;
    177         this._formattedLineEndings.pop();
    178192    }
    179193

  • trunk/Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js

    r183331 r184000  
    11/*
    22 * Copyright (C) 2013 Apple Inc. All rights reserved.
     3 * Copyright (C) 2015 Tobias Reiss <tobi+webkit@basecode.de>
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    777778    }
    778779
     780    _formattedContentFromEditor()
     781    {
     782        var mapping = {original: [0], formatted: [0]};
     783        // FIXME: <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector
     784        var indentString = "    ";
     785        var builder = new FormatterContentBuilder(mapping, [], [], 0, 0, indentString);
     786        var formatter = new Formatter(this._codeMirror, builder);
     787        var start = {line: 0, ch: 0};
     788        var end = {line: this._codeMirror.lineCount() - 1};
     789        formatter.format(start, end);
     790
     791        return builder.formattedContent.trim();
     792    }
     793
    779794    _resetContent()
    780795    {
    … …  
    817832            var selectionAnchor = this._codeMirror.getCursor("anchor");
    818833            var selectionHead = this._codeMirror.getCursor("head");
    819 
    820             function countNewLineCharacters(text)
    821             {
    822                 var matches = text.match(/\n/g);
    823                 return matches ? matches.length : 0;
     834            var isEditorReadOnly = this._codeMirror.getOption("readOnly");
     835            var styleText = this._style.text.trim();
     836            var findWhitespace = /\s+/g;
     837
     838            // Only format non-empty styles. Keep in mind that styleText is always empty
     839            // for "readOnly" Editors. But prepare Checkbox placeholders in any case.
     840            // Because that will indent the cursor when the User starts typing.
     841            if (!styleText && !isEditorReadOnly) {
     842                this._markLinesWithCheckboxPlaceholder();
     843                return;
    824844            }
    825845
    826             var styleText = this._style.text;
    827 
    828             // Pretty print the content if there are more properties than there are lines.
    829             // This could be an option exposed to the user; however, it is almost always
    830             // desired in this case.
    831 
    832             if (styleText && this._style.visibleProperties.length <= countNewLineCharacters(styleText.trim()) + 1) {
    833                 // This style has formatted text content, so use it for a high-fidelity experience.
    834 
    835                 var prefixWhitespaceMatch = styleText.match(/^[ \t]*\n/);
    836                 this._prefixWhitespace = prefixWhitespaceMatch ? prefixWhitespaceMatch[0] : "";
    837 
    838                 var suffixWhitespaceMatch = styleText.match(/\n[ \t]*$/);
    839                 this._suffixWhitespace = suffixWhitespaceMatch ? suffixWhitespaceMatch[0] : "";
    840 
    841                 this._codeMirror.setValue(styleText);
    842 
    843                 if (this._prefixWhitespace)
    844                     this._codeMirror.replaceRange("", {line: 0, ch: 0}, {line: 1, ch: 0});
    845 
    846                 if (this._suffixWhitespace) {
    847                     var lineCount = this._codeMirror.lineCount();
    848                     this._codeMirror.replaceRange("", {line: lineCount - 2}, {line: lineCount - 1});
     846            // Set non-optimized, valid and invalid styles in preparation for the Formatter.
     847            // Set empty string in case of readonly styles.
     848            this._codeMirror.setValue(styleText);
     849
     850            if (isEditorReadOnly) {
     851                var lineNumber = 0;
     852                this._iterateOverProperties(false, function(property) {
     853                    var from = {line: lineNumber, ch: 0};
     854                    var to = {line: lineNumber};
     855                    // Readonly properties are pretty printed by `synthesizedText` and not the Formatter.
     856                    this._codeMirror.replaceRange((lineNumber ? "\n" : "") + property.synthesizedText, from);
     857                    this._createTextMarkerForPropertyIfNeeded(from, to, property);
     858                    lineNumber++;
     859                });
     860
     861                return;
     862            }
     863
     864            // Now the Formatter pretty prints the styles.
     865            this._codeMirror.setValue(this._formattedContentFromEditor());
     866
     867            // We need to workaround the fact that...
     868            // 1) `this._style.properties` only holds valid CSSProperty instances but not
     869            // comments and invalid properties like `color;`.
     870            // 2) `_createTextMarkerForPropertyIfNeeded` relies on CSSProperty instances.
     871            var cssPropertiesMap = new Map();
     872            this._iterateOverProperties(false, function(cssProperty) {
     873                cssPropertiesMap.set(cssProperty.text.replace(findWhitespace, ""), cssProperty);
     874            });
     875
     876            // Go through the Editor line by line and create TextMarker when a
     877            // CSSProperty instance for that property exists. If not, then don't create a TextMarker.
     878            this._codeMirror.eachLine(function(lineHandler) {
     879                var lineNumber = lineHandler.lineNo();
     880                var lineContentSansWhitespace = lineHandler.text.replace(findWhitespace, "");
     881                if (cssPropertiesMap.has(lineContentSansWhitespace)) {
     882                    var from = {line: lineNumber, ch: 0};
     883                    var to = {line: lineNumber};
     884                    this._createTextMarkerForPropertyIfNeeded(from, to, cssPropertiesMap.get(lineContentSansWhitespace));
    849885                }
    850 
    851                 this._linePrefixWhitespace = "";
    852 
    853                 var linesToStrip = [];
    854 
    855                 // Remember the whitespace so it can be restored on commit.
    856                 var lineCount = this._codeMirror.lineCount();
    857                 for (var i = 0; i < lineCount; ++i) {
    858                     var lineContent = this._codeMirror.getLine(i);
    859                     var prefixWhitespaceMatch = lineContent.match(/^\s+/);
    860 
    861                     // If there is no prefix whitespace (except for empty lines) then the prefix
    862                     // whitespace of all other lines will be retained as is. Update markers and return.
    863                     if (!prefixWhitespaceMatch) {
    864                         if (!lineContent)
    865                             continue;
    866                         this._linePrefixWhitespace = "";
    867                         this._updateTextMarkers(true);
    868                         return;
    869                     }
    870 
    871                     linesToStrip.push(i);
    872 
    873                     // Only remember the shortest whitespace so we don't loose any of the
    874                     // original author's whitespace if their indentation lengths differed.
    875                     // Using the shortest also makes the adjustment work in _updateTextMarkers.
    876 
    877                     // FIXME: This messes up if there is a mix of spaces and tabs. A tab
    878                     // is treated the same as a space when prefix whitespace is omitted,
    879                     // so if the shortest prefixed whitespace is, say, two tab characters,
    880                     // lines that begin with four spaces will only have a two space indent.
    881                     if (!this._linePrefixWhitespace || prefixWhitespaceMatch[0].length < this._linePrefixWhitespace.length)
    882                         this._linePrefixWhitespace = prefixWhitespaceMatch[0];
    883                 }
    884 
    885                 // Strip the whitespace from the beginning of each line.
    886                 for (var i = 0; i < linesToStrip.length; ++i) {
    887                     var lineNumber = linesToStrip[i];
    888                     var from = {line: lineNumber, ch: 0};
    889                     var to = {line: lineNumber, ch: this._linePrefixWhitespace.length};
    890                     this._codeMirror.replaceRange("", from, to);
    891                 }
    892 
    893                 // Update all the text markers.
    894                 this._updateTextMarkers(true);
    895             } else {
    896                 // This style does not have text content or it is minified, so we want to synthesize the text content.
    897 
    898                 this._prefixWhitespace = "";
    899                 this._suffixWhitespace = "";
    900                 this._linePrefixWhitespace = "";
    901 
    902                 this._codeMirror.setValue("");
    903 
    904                 var lineNumber = 0;
    905 
    906                 // Iterate only visible properties if we have original style text. That way we known we only synthesize
    907                 // what was originaly in the style text.
    908                 this._iterateOverProperties(styleText ? true : false, function(property) {
    909                     // Some property text can have line breaks, so consider that in the ranges below.
    910                     var propertyText = property.synthesizedText;
    911                     var propertyLineCount = countNewLineCharacters(propertyText);
    912 
    913                     var from = {line: lineNumber, ch: 0};
    914                     var to = {line: lineNumber + propertyLineCount};
    915 
    916                     this._codeMirror.replaceRange((lineNumber ? "\n" : "") + propertyText, from);
    917                     this._createTextMarkerForPropertyIfNeeded(from, to, property);
    918 
    919                     lineNumber += propertyLineCount + 1;
    920                 });
    921 
    922                 // Look for colors and make swatches.
    923                 this._createColorSwatches(true);
    924             }
    925 
    926             this._markLinesWithCheckboxPlaceholder();
     886            }.bind(this));
     887
     888            // Look for colors and make swatches.
     889            this._createColorSwatches(true);
    927890
    928891            // Restore the cursor position/selection.
    … …  
    935898            // Mark the editor as clean (unedited state).
    936899            this._codeMirror.markClean();
     900
     901            this._markLinesWithCheckboxPlaceholder();
    937902        }
    938903

  • trunk/Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js

    r183043 r184000  
    11/*
    22 * Copyright (C) 2013 Apple Inc. All rights reserved.
     3 * Copyright (C) 2015 Tobias Reiss <tobi+webkit@basecode.de>
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    105106
    106107        return 0;
     108    },
     109
     110    removeLastWhitespace: function(lastToken, lastContent, token, state, content, isComment)
     111    {
     112        return false;
    107113    },
    108114
    … …  
    375381    },
    376382
     383    removeLastWhitespace: function(lastToken, lastContent, token, state, content, isComment)
     384    {
     385        return false;
     386    },
     387
    377388    removeLastNewline: function(lastToken, lastContent, token, state, content, isComment, firstTokenOnLine)
    378389    {
    … …  
    420431    }
    421432});
     433
     434CodeMirror.extendMode("css-rule", {
     435    shouldHaveSpaceBeforeToken: function(lastToken, lastContent, token, state, content, isComment)
     436    {
     437        return lastContent === ":" && !lastToken;
     438    },
     439
     440    shouldHaveSpaceAfterLastToken: function(lastToken, lastContent, token, state, content, isComment)
     441    {
     442        return lastContent === "," && !lastToken;
     443    },
     444
     445    newlinesAfterToken: function(lastToken, lastContent, token, state, content, isComment)
     446    {
     447        return 0;
     448    },
     449
     450    removeLastWhitespace: function(lastToken, lastContent, token, state, content, isComment)
     451    {
     452        // Remove whitespace before a comment which moves the comment to the beginning of the line.
     453        if (isComment)
     454            return true;
     455
     456        // A semicolon indicates the end of line. So remove whitespace before next line.
     457        if (!lastToken)
     458            return lastContent === ";";
     459
     460        // Remove whitespace before semicolon. Like `prop: value ;`.
     461        // Remove whitespace before colon. Like `prop : value;`.
     462        if (!token)
     463            return content === ";" || content === ":";
     464
     465        // A comment is supposed to be in its own line. So remove whitespace before next line.
     466        if (/\bcomment\b/.test(lastToken))
     467            return true;
     468
     469        return false;
     470    },
     471
     472    removeLastNewline: function(lastToken, lastContent, token, state, content, isComment, firstTokenOnLine)
     473    {
     474        return false;
     475    },
     476
     477    indentAfterToken: function(lastToken, lastContent, token, state, content, isComment)
     478    {
     479        return false;
     480    },
     481
     482    newlineBeforeToken: function(lastToken, lastContent, token, state, content, isComment)
     483    {
     484        // Add new line before comments.
     485        if (isComment)
     486            return true;
     487
     488        // Add new line before a prefixed property like `-webkit-animation`.
     489        if (state.state === "block")
     490            return /\bmeta\b/.test(token);
     491
     492        // Add new line after comment
     493        if (/\bcomment\b/.test(lastToken))
     494            return true;
     495
     496        // Add new line before a regular property like `display`.
     497        if (/\bproperty\b/.test(token))
     498            return !(/\bmeta\b/.test(lastToken));
     499
     500        return false;
     501    },
     502
     503    indentBeforeToken: function(lastToken, lastContent, token, state, content, isComment)
     504    {
     505        return false;
     506    },
     507
     508    dedentsBeforeToken: function(lastToken, lastContent, token, state, content, isComment)
     509    {
     510        return 0;
     511    }
     512});
Note: See TracChangeset for help on using the changeset viewer.