Bug 167949 – Web Inspector: Option+hover on -webkit-transform in Styles sidebar underlines only half the property

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Via Antoine Quint:

"When you have a -wekit-transform property in the Styles sidebar and you hold down Cmd while hovering over it, the text is only partially underlined as the string is split in two tokens."

[Blaze Burg]

<rdar://problem/14057565>

[Blaze Burg]

Joe said in radar:

"So this is because CodeMirror is splitting it up into 2 tokens, 1 vendor prefix and 1 actual property. We can make CodeMirrorTokenTrackingController smarter here, and treat both as 1 "token" for the Cmd+Hover mode."

[Devin Rousso]

Created attachment 300969 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 300969 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:351
> +        let preceedingTokenPosition = Object.shallowCopy(position);
> +        preceedingTokenPosition.ch = tokenInfo.token.start - 1;

This might need to sanity check some things: What if tokenInfo.token.start was 0. Then this would set ch to -1. Would that cause a problem?

[Devin Rousso]

Comment on attachment 300969 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:351
>> +        preceedingTokenPosition.ch = tokenInfo.token.start - 1;
> 
> This might need to sanity check some things: What if tokenInfo.token.start was 0. Then this would set ch to -1. Would that cause a problem?

So I just tried this, and it appears to work fine.  I think that CodeMirror will create an "empty" token if the position is invalid.  Either way, I also check that the token exists and has a type, so it should cover both cases.

[Joseph Pecoraro]

Comment on attachment 300969 [details]
Patch

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

r=me

>>> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:351
>>> +        preceedingTokenPosition.ch = tokenInfo.token.start - 1;
>> 
>> This might need to sanity check some things: What if tokenInfo.token.start was 0. Then this would set ch to -1. Would that cause a problem?
> 
> So I just tried this, and it appears to work fine.  I think that CodeMirror will create an "empty" token if the position is invalid.  Either way, I also check that the token exists and has a type, so it should cover both cases.

I just want to make sure it doesn't throw an exception.  I see now that CodeMirror clips positions to ensure they are in bounds (clipPos and clipToLen).

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:354
> +        if (preceedingToken && preceedingToken.type && preceedingToken.type.includes("meta")) {

Includes "meta" could false trigger if the type was something was something like "not-meta". But we seem to do this style elsewhere and meta is pretty unique =).

[Devin Rousso]

Created attachment 301027 [details]
Patch

So I just realized that I forgot to include a check for if the user highlights the meta token instead of the non-meta token.  These changes now cover both.

[Joseph Pecoraro]

Comment on attachment 301027 [details]
Patch

Nice. r=me

[WebKit Commit Bot]

Comment on attachment 301027 [details]
Patch

Clearing flags on attachment: 301027

Committed r211973: <http://trac.webkit.org/changeset/211973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.