Bug 146676 – Web Inspector: Tabbing in the styles sidebar doesn't highlight the next section of text

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Created attachment 256284 [details]
Actual

This occurs when the value of the property can be entirely found in the name of that property.  For example, "border-collapse: collapse;" will highlight the "border-collapse" and then just the "collapse" because the code is trying to find the first index of the value (which is "collapse") and that happens to be inside the name.

Thanks to @Nikita for the gif.

[Radar WebKit Bug Importer]

<rdar://problem/21699601>

[Devin Rousso]

Created attachment 256286 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 256286 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:400
> +        var substringIndex = text.includes(":") && cursor.ch >= text.indexOf(":") ? text.indexOf(":") : 0;

Nit: text.indexOf(":") used twice here, it should be pulled into a variable.

[Devin Rousso]

Created attachment 256288 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256288 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:400
> +        var colonIndex = text.indexOf(":");

What if this returns -1?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:403
> +        var match = /(?:[^:;\s]\s*)+/.exec(remaining);

You can do this without doing a substring. If you make the regex, then set regex.lastIndex = colonIndex, it will only search after that.

[Devin Rousso]

Comment on attachment 256288 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:400
>> +        var colonIndex = text.indexOf(":");
> 
> What if this returns -1?

On the next line, it checks if colonIndex >= 0 and if not then set the start to 0 since the entire string should be matched.

[Timothy Hatcher]

Comment on attachment 256288 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:400
>>> +        var colonIndex = text.indexOf(":");
>> 
>> What if this returns -1?
> 
> On the next line, it checks if colonIndex >= 0 and if not then set the start to 0 since the entire string should be matched.

Oh, I see it now.

[Devin Rousso]

Created attachment 256307 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256307 [details]
Patch

Clearing flags on attachment: 256307

Committed r186468: <http://trac.webkit.org/changeset/186468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.