In the past few versions enabling spreadsheet experiment didn't really work, but looks like in latest version its starting working to wanted to provide some feedback. In Chrome if you enter a ";" it closes that line and moves to the next. With Webkit/Safari it does not. In fact it lets you add multiple ";" or add one and still has a faded out gray one. Should always only show one, and if you enter a ";" should close that rule and move to next line.
Thanks for the feedback. I haven't yet implemented special handling of semicolons in values, but I'm planning to.
<rdar://problem/35065995>
Created attachment 328330 [details] Patch
Comment on attachment 328330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328330&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:262 > + const quoteRegex = /["']/g; > + let value = this.value; > + let start = -1; > + let end = value.length; > + let match = null; > + while (match = quoteRegex.exec(value)) { > + if (start < 0) > + start = match.index; > + end = match.index + 1; > + } > + > + if (value.substring(start, end).hasMatchingEscapedQuotes()) { > + commit(); > + return; > + } I see what this is trying to do but I don't think it is doing it correctly. Right now it is just scanning for quotes, and doesn't detect if those quotes are within quotes. I think the algorithm you want is something like this: if (event.key === ";") { // We only want to commit on semicolon if the semicolon was at the end of the value. let semicolonInsertionIndex = window.getSelection().getRangeAt(0).startOffset; if (semicolonInsertionIndex !== this.value.length) return; // Determine if the semicolon is inside of an unfinished string. // First gather all complete quoted strings. Then check at the // end for an unfinished string. let range; let startIndex = 0; while (range = findQuotedStringRange(value, startIndex)) startIndex = range.endIndex + 1; range = findUnterminatedStringRange(value, startIndex); if (range) { // Assert that the unterminated string range does go to the end of the string (where the semicolon is). console.assert(range.endIndex === value.length - 1); return; } // We have now determined this is a semicolon, not inside of an unterminated string // and at the end of input. Lets go! commit(); return; } With findQuotedStringRange / findUnterminatedStringRange left as an exercise for the reader. That make sense?
Comment on attachment 328330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328330&action=review > Source/WebInspectorUI/ChangeLog:12 > + semicolon after `url("foo` or `url(bar` would not move to the next property. `url(bar` doesn't have any quotes. I don't see in your code handling of parenthesis.
Comment on attachment 328330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328330&action=review >> Source/WebInspectorUI/ChangeLog:12 >> + semicolon after `url("foo` or `url(bar` would not move to the next property. > > `url(bar` doesn't have any quotes. I don't see in your code handling of parenthesis. I don't think we have to worry about parenthesis. I was only worried about quotes for situations like `content: ";";`, as the first semicolon is a string, not a property "terminator" (not sure what the word is). >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:262 >> + } > > I see what this is trying to do but I don't think it is doing it correctly. Right now it is just scanning for quotes, and doesn't detect if those quotes are within quotes. > > I think the algorithm you want is something like this: > > if (event.key === ";") { > // We only want to commit on semicolon if the semicolon was at the end of the value. > let semicolonInsertionIndex = window.getSelection().getRangeAt(0).startOffset; > if (semicolonInsertionIndex !== this.value.length) > return; > > // Determine if the semicolon is inside of an unfinished string. > // First gather all complete quoted strings. Then check at the > // end for an unfinished string. > let range; > let startIndex = 0; > while (range = findQuotedStringRange(value, startIndex)) > startIndex = range.endIndex + 1; > range = findUnterminatedStringRange(value, startIndex); > if (range) { > // Assert that the unterminated string range does go to the end of the string (where the semicolon is). > console.assert(range.endIndex === value.length - 1); > return; > } > > // We have now determined this is a semicolon, not inside of an unterminated string > // and at the end of input. Lets go! > commit(); > return; > } > > With findQuotedStringRange / findUnterminatedStringRange left as an exercise for the reader. > > That make sense? I don't think we need to detect quotes within quotes. All I really care about is that there are no unbalanced quotes, as in that case the semicolon could be part of the value instead of the property "terminator". // OK url(abc); url("abc"); url('abc'); url("a'b'c"); url('a"b"c'); url("a\"b\"c"); url('a\'b\'c'); // Bad url("ab url('ab url("a"b" url('a'b' url("a"b' url('a'b" url("a"'b' url('a'"b" The purpose of the while loop is to find the index (if any) of the first and last quote character (" or ') so that I can then check that substring to ensure that it is properly "wrapped". If it is, then there's no way that the semicolon could be part of a string, so we can assume that it's the property "terminator".
Comment on attachment 328330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328330&action=review >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:262 >>> + } >> >> I see what this is trying to do but I don't think it is doing it correctly. Right now it is just scanning for quotes, and doesn't detect if those quotes are within quotes. >> >> I think the algorithm you want is something like this: >> >> if (event.key === ";") { >> // We only want to commit on semicolon if the semicolon was at the end of the value. >> let semicolonInsertionIndex = window.getSelection().getRangeAt(0).startOffset; >> if (semicolonInsertionIndex !== this.value.length) >> return; >> >> // Determine if the semicolon is inside of an unfinished string. >> // First gather all complete quoted strings. Then check at the >> // end for an unfinished string. >> let range; >> let startIndex = 0; >> while (range = findQuotedStringRange(value, startIndex)) >> startIndex = range.endIndex + 1; >> range = findUnterminatedStringRange(value, startIndex); >> if (range) { >> // Assert that the unterminated string range does go to the end of the string (where the semicolon is). >> console.assert(range.endIndex === value.length - 1); >> return; >> } >> >> // We have now determined this is a semicolon, not inside of an unterminated string >> // and at the end of input. Lets go! >> commit(); >> return; >> } >> >> With findQuotedStringRange / findUnterminatedStringRange left as an exercise for the reader. >> >> That make sense? > > I don't think we need to detect quotes within quotes. All I really care about is that there are no unbalanced quotes, as in that case the semicolon could be part of the value instead of the property "terminator". > > // OK > url(abc); > url("abc"); > url('abc'); > url("a'b'c"); > url('a"b"c'); > url("a\"b\"c"); > url('a\'b\'c'); > > // Bad > url("ab > url('ab > url("a"b" > url('a'b' > url("a"b' > url('a'b" > url("a"'b' > url('a'"b" > > The purpose of the while loop is to find the index (if any) of the first and last quote character (" or ') so that I can then check that substring to ensure that it is properly "wrapped". If it is, then there's no way that the semicolon could be part of a string, so we can assume that it's the property "terminator". Hmm. Then this deserves a comment in the code at this point to inform the reader about what it is trying to do. Since I saw this, assumed it was trying to do something else, and came to different conclusions. In the end I do think a simple sanity check makes sense. As NVI pointed out, top level parenthesis throw a wrench in really parsing (and I hadn't even considered them). I'm fine with a simple check as long as there is a comment describing what it is doing, because it isn't intended to be a 100% perfect solution and should say so.
Comment on attachment 328330 [details] Patch r- because this patch doesn't work for values without any quotes, e.g.: color: red; I think typing ; only in property value should have a special case. Typing ; in property name shouldn't navigate to property value. I implemented the special case for ":" (https://trac.webkit.org/changeset/224906/webkit) in SpreadsheetStyleProperty and not SpreadsheetTextField because I didn't want to add any CSS property logic to SpreadsheetTextField, so I could (hopefully) merge SpreadsheetTextField with SpreadsheetSelectorField later.
Created attachment 329164 [details] Patch
Comment on attachment 329164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329164&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:513 > + let text = this._valueElement.textContent; > + if (window.getSelection().getRangeAt(0).startOffset !== text.length) > + return; Typing ; should move to the next property even if it's before the suggestion hint. Consider: float: r[ight] [irht] is a suggestion hint here. Typing ; should move the focus to the next property. SpreadsheetTextField.prototype._getPrefix returns text without the suggestion hint. You can make it public and possibly rename to something more meaningful.
Created attachment 329433 [details] Patch
Comment on attachment 329433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329433&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:511 > + if (window.getSelection().getRangeAt(0).startOffset !== text.length) 1. When the whole value is selected, I'd expect typing ";" to move to the next field. This can be done by replacing startOffset with endOffset. 2. Usually, you want to check getSelection().rangeCount before calling getRangeAt. When nothing is selected, getRangeAt throws an exception: (DOM Exception 1): The index is not in the allowed range. I don't know if it ever happens here.
Created attachment 329677 [details] Patch
Comment on attachment 329677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329677&action=review r=me > Source/WebInspectorUI/ChangeLog:13 > + semicolon after `url("foo` or `url(bar` would not move to the next property. I don't see how it would make `url(bar;` not move to the next property. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528 > + if (!quoteRegex.test(text) || text.substring(start, end).hasMatchingEscapedQuotes()) { No need to run the regex again, just check if we found a match: if (start === -1 || ...) Better yet make this an early return: if (start !== -1 && !text.substring(start, end).hasMatchingEscapedQuotes()) return;
Comment on attachment 329677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329677&action=review >> Source/WebInspectorUI/ChangeLog:13 >> + semicolon after `url("foo` or `url(bar` would not move to the next property. > > I don't see how it would make `url(bar;` not move to the next property. Oops. This was supposed to be `url('bar;`.
Created attachment 329831 [details] Patch
Comment on attachment 329831 [details] Patch Clearing flags on attachment: 329831 Committed r226149: <https://trac.webkit.org/changeset/226149>
All reviewed patches have been landed. Closing bug.