179587 – Web Inspector: Styles Redesign: can't add new property after property without trailing semicolon

RESOLVED FIXED 179587

Web Inspector: Styles Redesign: can't add new property after property without trailing semicolon

https://bugs.webkit.org/show_bug.cgi?id=179587

Summary Web Inspector: Styles Redesign: can't add new property after property without...

Nikita Vasilyev

Reported 2017-11-11 19:26:56 PST

Created attachment 326703 [details] [Animated GIF] Bug Steps: 1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/no-trailing-semicolon.html 2. Inspect <body>. 3. Click on the white space after "color: #333". 4. Add any property, such as "font-size: 12px". Expected: "font-size: 12px" property gets added. Actual: "color: #333" no longer works and the newly added property doesn't work either. Notes: When there's no ";" after the last property, it doesn't get added, resulting in this: color: #333font-size: 12px;

Attachments
[Animated GIF] Bug (173.23 KB, image/gif) 2017-11-11 19:26 PST, Nikita Vasilyev	no flags	Details
Patch (2.24 KB, patch) 2017-11-27 15:00 PST, Nikita Vasilyev	timothy: review-	Details Formatted Diff Diff
[Animated GIF] With patch applied (155.83 KB, image/gif) 2017-11-27 15:04 PST, Nikita Vasilyev	no flags	Details
Patch (2.12 KB, patch) 2017-11-29 14:00 PST, Nikita Vasilyev	timothy: review+	Details Formatted Diff Diff
Patch (2.13 KB, patch) 2017-11-29 14:43 PST, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-11-11 19:27:17 PST

<rdar://problem/35490858>

Nikita Vasilyev

Comment 2 2017-11-27 15:00:23 PST

Created attachment 327689 [details] Patch

Nikita Vasilyev

Comment 3 2017-11-27 15:04:37 PST

Created attachment 327691 [details] [Animated GIF] With patch applied

Timothy Hatcher

Comment 4 2017-11-28 14:58:46 PST

You should add a space after the semicolon too.

Timothy Hatcher

Comment 5 2017-11-28 15:05:33 PST

Comment on attachment 327689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327689&action=review > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382 > + let trimmedText = styleText.trimRight(); > + if (!trimmedText) > + return styleText; > + > + if (trimmedText.slice(-1) === ";") Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:384 > + else No need for this else after a return. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:385 > + return styleText + ";"; Add a space with the semicolon too?

Nikita Vasilyev

Comment 6 2017-11-28 15:22:27 PST

Comment on attachment 327689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327689&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382 >> + if (trimmedText.slice(-1) === ";") > > Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead. If I use lastIndexOf I'd have to check if the text after ";" is a white space. Consider: font-size:12px; color: red Maybe I should just use a simple RegExp? if (/;\s*$/.test(styleText)) >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:385 >> + return styleText + ";"; > > Add a space with the semicolon too? I'll add a space in this patch, and I'll add proper indentation in Bug 178835 - Web Inspector: [PARITY] Styles Redesign: Add indentation set in settings for newly added properties.

Nikita Vasilyev

Comment 7 2017-11-28 18:54:24 PST

(In reply to Nikita Vasilyev from comment #6) > Comment on attachment 327689 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327689&action=review > > >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382 > >> + if (trimmedText.slice(-1) === ";") > > > > Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead. > > If I use lastIndexOf I'd have to check if the text after ";" is a white > space. Consider: > > font-size:12px; > color: red > > Maybe I should just use a simple RegExp? > > if (/;\s*$/.test(styleText)) After looking at it more, I couldn't make this function any simpler by using lastIndexOf. Consider the following cases: "\n " -> "\n " "color: red" -> "color: red; " "color: red;" -> "color: red;" "font-size: 12px; color: red" -> "font-size: 12px; color: red; " trimmedText.slice(-1) returns the last character. It shouldn't be an expensive operation.

Timothy Hatcher

Comment 8 2017-11-29 10:20:51 PST

Comment on attachment 327689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327689&action=review >>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382 >>>> + if (trimmedText.slice(-1) === ";") >>> >>> Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead. >> >> If I use lastIndexOf I'd have to check if the text after ";" is a white space. Consider: >> >> font-size:12px; >> color: red >> >> Maybe I should just use a simple RegExp? >> >> if (/;\s*$/.test(styleText)) > > After looking at it more, I couldn't make this function any simpler by using lastIndexOf. Consider the following cases: > > "\n " -> "\n " > "color: red" -> "color: red; " > "color: red;" -> "color: red;" > "font-size: 12px; color: red" -> "font-size: 12px; color: red; " > > trimmedText.slice(-1) returns the last character. It shouldn't be an expensive operation. You are right, I wasn't thinking about whitespace. I think regex would be best then. The initial trimRight() is what I was wanting to avoid.

Nikita Vasilyev

Comment 9 2017-11-29 14:00:36 PST

Created attachment 327901 [details] Patch

Nikita Vasilyev

Comment 10 2017-11-29 14:04:54 PST

Comment on attachment 327901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327901&action=review > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:378 > + if (/[^;\s]\s*$/.test(styleText)) I inverted the logic in the RegExp, since if (!/;\s*$/.test(styleText) this would add a semicolon to "\n ".

Timothy Hatcher

Comment 11 2017-11-29 14:19:18 PST

Comment on attachment 327901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327901&action=review > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:379 > + return styleText + "; "; In this case I think return styleText.trimRight() + "; "; would be better then. And it only trims if needed.

Nikita Vasilyev

Comment 12 2017-11-29 14:43:10 PST

Created attachment 327910 [details] Patch

WebKit Commit Bot

Comment 13 2017-11-29 15:16:23 PST

Comment on attachment 327910 [details] Patch Clearing flags on attachment: 327910 Committed r225299: <https://trac.webkit.org/changeset/225299>

WebKit Commit Bot

Comment 14 2017-11-29 15:16:25 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Nikita Vasilyev

Reported

2017-11-11 19:26 PST

Modified

2017-11-29 15:16 PST History

CC List

4 users Show

URL

Keywords InRadar

Depends on

Blocks