Bug 179587

Summary:

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

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, inspector-bugzilla-changes, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[Animated GIF] Bug	none
Patch	timothy: review-
[Animated GIF] With patch applied	none
Patch	timothy: review+
Patch	none

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.