Bug 119012 – Web Inspector: pressing the Cmd key over a CSS property should underline it immediately (jump to definition mode)

• Component: Web Inspector
• Status: RESOLVED FIXED

[Antoine Quint]

When the user mouses over a CSS property in the styles panel and pressed the command key, it should underline directly so that it's clear that jump-to-definition is available. Right now, you need to move the mouse before the Cmd key press is registered.

[Radar WebKit Bug Importer]

<rdar://problem/14521089>

[Antoine Quint]

Created attachment 213249 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 213249 [details]
Patch

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

I think you can remove a bunch of code here now right? If CodeMirrorTokenTracking controller is always tracking when appropriate then:

    1. Make CodeMirrorTokenTrackingController.prototype.{start,stop}Tracking private, or roll them into _mouseEntered and _mouseLeft
    2. Remove CodeMirrorTokenTrackingController.prototype.tracking and code that uses it
      - Remove SourceCodeTextEditor.prototype._shouldTrackTokenHovering, _startTrackingTokenHoveringIfNeeded, _stopTrackingTokenHoveringIfNeeded, _debuggerDidPause
      - Remove registration and callers of _debuggerDidPause

The only thing I am unsure of is the bottom two lines of this patch (if we disable jump to symbol tracking by unpressing Cmd, the removal highlighted range). I suspect you can just remove the if tracking branch.

I'd like to see a follow up patch removing the dead code!

> Source/WebInspectorUI/ChangeLog:22
> +        tracking since otherwise pressing the Cmd key over a read-only editor could highlight the previously selected
> +        candidate of another editor.

Is that really true? Each editor gets a separate tokenTrackingController instance, how could a range be highlighted in a separate editor?

> Source/WebInspectorUI/UserInterface/Main.js:110
> +    window.addEventListener("mousemove", this._mousemoved.bind(this), true);

Style: "_mousemoved" => "_mouseMoved"

[Antoine Quint]

(In reply to comment #3)
> (From update of attachment 213249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213249&action=review
> 
> I think you can remove a bunch of code here now right? If CodeMirrorTokenTracking controller is always tracking when appropriate then:
> 
>     1. Make CodeMirrorTokenTrackingController.prototype.{start,stop}Tracking private, or roll them into _mouseEntered and _mouseLeft
>     2. Remove CodeMirrorTokenTrackingController.prototype.tracking and code that uses it
>       - Remove SourceCodeTextEditor.prototype._shouldTrackTokenHovering, _startTrackingTokenHoveringIfNeeded, _stopTrackingTokenHoveringIfNeeded, _debuggerDidPause
>       - Remove registration and callers of _debuggerDidPause
> 
> The only thing I am unsure of is the bottom two lines of this patch (if we disable jump to symbol tracking by unpressing Cmd, the removal highlighted range). I suspect you can just remove the if tracking branch.
> 
> I'd like to see a follow up patch removing the dead code!

Most of the code I added ends up being specific to the NonSymbolTokens mode, I should see if I could shave off some code for the other mode too.

> > Source/WebInspectorUI/ChangeLog:22
> > +        tracking since otherwise pressing the Cmd key over a read-only editor could highlight the previously selected
> > +        candidate of another editor.
> 
> Is that really true? Each editor gets a separate tokenTrackingController instance, how could a range be highlighted in a separate editor?

Because the monitoring of key modifier changes is shared through all text editor instances. There's probably another way to deal with this though.

> > Source/WebInspectorUI/UserInterface/Main.js:110
> > +    window.addEventListener("mousemove", this._mousemoved.bind(this), true);
> 
> Style: "_mousemoved" => "_mouseMoved"

Thanks for spotting that.

[Joseph Pecoraro]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 213249 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213249&action=review
> > 
> > > Source/WebInspectorUI/ChangeLog:22
> > > +        tracking since otherwise pressing the Cmd key over a read-only editor could highlight the previously selected
> > > +        candidate of another editor.
> > 
> > Is that really true? Each editor gets a separate tokenTrackingController instance, how could a range be highlighted in a separate editor?
> 
> Because the monitoring of key modifier changes is shared through all text editor instances. There's probably another way to deal with this though.

But shouldn't your mouse only be over one editor, so only that editor will have a candidate / update.

Maybe this is indicative of a bug? Maybe on mouseLeft we need to clear something? It already clears the tracking state, so many something should respect the tracking state?

[Antoine Quint]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 213249 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=213249&action=review
> > > 
> > > > Source/WebInspectorUI/ChangeLog:22
> > > > +        tracking since otherwise pressing the Cmd key over a read-only editor could highlight the previously selected
> > > > +        candidate of another editor.
> > > 
> > > Is that really true? Each editor gets a separate tokenTrackingController instance, how could a range be highlighted in a separate editor?
> > 
> > Because the monitoring of key modifier changes is shared through all text editor instances. There's probably another way to deal with this though.
> 
> But shouldn't your mouse only be over one editor, so only that editor will have a candidate / update.
> 
> Maybe this is indicative of a bug? Maybe on mouseLeft we need to clear something? It already clears the tracking state, so many something should respect the tracking state?

Correct! I'll fix this in an updated patch coming up.

[Antoine Quint]

Created attachment 213357 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 213357 [details]
Patch

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

r=me, thanks for cleaning up the dead code!

> Source/WebInspectorUI/ChangeLog:46
> +        "mouseleave" event hanling when we're in the "enabled" state. Additionally, the public

Typo: "hanling" => "handling"

[Antoine Quint]

Created attachment 213398 [details]
Patch for landing

[Antoine Quint]

(In reply to comment #8)
> (From update of attachment 213357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213357&action=review
> 
> r=me, thanks for cleaning up the dead code!

My pleasure, thanks for the thoughtful and thorough review!

> > Source/WebInspectorUI/ChangeLog:46
> > +        "mouseleave" event hanling when we're in the "enabled" state. Additionally, the public
> 
> Typo: "hanling" => "handling"

Will be fixed in commit.

[WebKit Commit Bot]

Comment on attachment 213398 [details]
Patch for landing

Clearing flags on attachment: 213398

Committed r156923: <http://trac.webkit.org/changeset/156923>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.