Bug 170184 – Web Inspector: tabbing in Styles sidebar is broken when additional ":" and ";" are in the property value

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

If you add `background-image: url(https://webkit.org/wp-content/themes/webkit/images/webkit.svg);` to any style and try tabbing, the highlight will get stuck on "url(https".  This is most likely because the way it searches for ":" and ";" is very dumb, simply finding the first instance in the remaining text.

[Devin Rousso]

Created attachment 305618 [details]
Patch

[Matt Baker]

Comment on attachment 305618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305618&action=review

Since we have so many editing issues in the styles sidebar, I think we should break out the range matching into a utility method (that handles plain text, not CodeMirror) so we can write tests.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:408
> +    _rangeForNextNameOrValue(codeMirror, cursor, text) {

Style: put open brace on newline.

[Matt Baker]

Comment on attachment 305618 [details]
Patch

r- until we have tests

[Devin Rousso]

Created attachment 306562 [details]
Patch

[Matt Baker]

Comment on attachment 306562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306562&action=review

r=me, nice tests!

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:408
> +    _rangeForNextNameOrValue(codeMirror, cursor, text) {

Nit: put open brace on its own line.

> LayoutTests/inspector/unit-tests/text-utilities-expected.txt:21
> +PASS: Next highlight range of "foo: url(http://baz);" starting at index 5 is [5, 20]

A couple thoughts on the test output:

1) Highlight is a higher-level UI concept, and doesn't appear in the name of the utility method. It's confusing unless you know how this is going to be used in the UI. Maybe use name/value instead.
2) It is hard to tell from the resulting range bounds alone where an error occurred. What do you think about including the resulting substring in the output? Something like:

        Next name/value token in "foo: url(http://baz);" starting at index 5 is "url(http://baz)" (5, 20)

> LayoutTests/inspector/unit-tests/text-utilities.html:14
> +                InspectorTest.expectShallowEqual(WebInspector.rangeForNextCSSNameOrValue(text, index), expected, `Next highlight range of "${text}" starting at index ${index} is [${expected.from}, ${expected.to}]`);

Style: I think this would read a little better as:

function testValid(text, index, expected) {
    let actual = WebInspector.rangeForNextCSSNameOrValue(text, index);
    InspectorTest.expectShallowEqual(actual, expected, ...);
}

[Devin Rousso]

Created attachment 306647 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306647 [details]
Patch

Clearing flags on attachment: 306647

Committed r215170: <http://trac.webkit.org/changeset/215170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.