Bug 122893 – Web Inspector: Go to line keyboard command and dialog

• Component: Web Inspector
• Status: RESOLVED FIXED

[Antoine Quint]

<rdar://problem/14092546>

[Antoine Quint]

There should be a way to easily jump to a line number in a source code editor.

[Antoine Quint]

Created attachment 214362 [details]
Patch

[Antoine Quint]

Note the two FIXMEs in the patch:

- how can I play an error sound when the input in the dialog is incorrect?
- how can I override the Cmd+L keyboard shortcut that is handled by Safari by default?

[Antoine Quint]

Created attachment 214363 [details]
Screenshot of the go-to-line dialog showing over a web page's source code view

[Joseph Pecoraro]

Comment on attachment 214362 [details]
Patch

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

r=me, nice!

> Source/WebInspectorUI/ChangeLog:10
> +        bahavior in Chrome.

Typo: bahavior

> Source/WebInspectorUI/UserInterface/GoToLineDialog.css:38
> +    box-shadow: 1px 5px 20px rgba(0, 0, 0, 0.25);

Nit: Comparing to Xcode, the box-shadow is a little darker then yours and is primarily below the box. Though we don't need to always copy them, I think this shadow could be made a little darker.

> Source/WebInspectorUI/UserInterface/GoToLineDialog.css:55
> +    font-family: "Lucida Grande";

Nit: We have to worry about other ports that might not have Lucida Grande. Also font-family should always end with a default keyword (serif, sans-serif, monospace, etc).

Just searching "Lucida Grande" in the inspector typically looks like:

    NavigationSidebarPanel.css
    85:    font-family: "Lucida Grande", sans-serif;
    108:    font-family: "Lucida Grande", "Helvetica", sans-serif;

> Source/WebInspectorUI/UserInterface/GoToLineDialog.css:78
> +.go-to-line-dialog img:active {

Nit: Add "> div > img" to the selector

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:61
> +        this._input.focus();

Xcode has a weird behavior where if you push ⌘L while the dialog is already showing it (in a seemingly ugly way) clears the dialog.

Should we do that? I would vote yes. If we do, we should have a this._clear() here, instead of in dismiss.

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:81
> +        console.log(event.type);

Nit: Stray debug log!

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:117
> +            else
> +                this._clear();

I like your behavior better then Xcode. Xcode seems to dismiss on Escape when non-empty, but here you just clear.

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:132
> +            // FIXME: This should play an error sound.

You can easily create InspectorFrontendHost.systemBeep. In WebCore the implementation would just call systemBeep() in WebCore/platform/Sound.h.

As with any InspectorFrontendHost APIs you'd have to update Safari's WebInspector.framework as well. it would be just a few lines following a common pattern. It would call NSBeep().

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:71
> +    // FIXME: Cmd+L shorcut doesn't actually work.
> +    new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command, "L", this.showGoToLineDialog.bind(this), this.element);

I don't have an answer for this right now. If the KeyboardShortcut event listener is setup in the capturing phase, does the web content event listener even get called, or does Safari handle it before us?

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:198
> +        if (!this._goToLineDialog)
> +            this._goToLineDialog = new WebInspector.GoToLineDialog;
> +
> +        this._goToLineDialog.delegate = this;

Put the delegate assignment inside the lazy instantiation.

> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:210
> +        var range = new WebInspector.TextRange(lineNumber - 1, 0, lineNumber, 0);

Clever! Does this work for the last line as well (I assume it will, I'd expect CodeMirror to clamp appropriately).

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:132
> > +            // FIXME: This should play an error sound.
> 
> You can easily create InspectorFrontendHost.systemBeep. In WebCore the implementation would just call systemBeep() in WebCore/platform/Sound.h.

Oh, and by no means do you have to follow my naming suggestion. Just "beep" might be better. You can name it whatever you think is best. Worst case, we could trigger an <audio> beep ourselves, but we should prefer the platform's beep sound.

[Timothy Hatcher]

Comment on attachment 214362 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/GoToLineDialog.css:38
>> +    box-shadow: 1px 5px 20px rgba(0, 0, 0, 0.25);
> 
> Nit: Comparing to Xcode, the box-shadow is a little darker then yours and is primarily below the box. Though we don't need to always copy them, I think this shadow could be made a little darker.

I agree. Maybe box-shadow: 1px 5px 20px 3px rgba(0, 0, 0, 0.33); would help. (Wild guess that I didn't even test.)

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 214362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214362&action=review
> 
> > Source/WebInspectorUI/UserInterface/GoToLineDialog.css:38
> > +    box-shadow: 1px 5px 20px rgba(0, 0, 0, 0.25);
> 
> Nit: Comparing to Xcode, the box-shadow is a little darker then yours and is primarily below the box. Though we don't need to always copy them, I think this shadow could be made a little darker.

Absolutely, I'll try Tim's suggestion.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.css:55
> > +    font-family: "Lucida Grande";
> 
> Nit: We have to worry about other ports that might not have Lucida Grande. Also font-family should always end with a default keyword (serif, sans-serif, monospace, etc).
> 
> Just searching "Lucida Grande" in the inspector typically looks like:
> 
>     NavigationSidebarPanel.css
>     85:    font-family: "Lucida Grande", sans-serif;
>     108:    font-family: "Lucida Grande", "Helvetica", sans-serif;

Good point.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.css:78
> > +.go-to-line-dialog img:active {
> 
> Nit: Add "> div > img" to the selector

Thanks, that was my intention.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:61
> > +        this._input.focus();
> 
> Xcode has a weird behavior where if you push ⌘L while the dialog is already showing it (in a seemingly ugly way) clears the dialog.
> 
> Should we do that? I would vote yes. If we do, we should have a this._clear() here, instead of in dismiss.

I hadn't caught that one, I'll implement this.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:81
> > +        console.log(event.type);
> 
> Nit: Stray debug log!

Oops.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:117
> > +            else
> > +                this._clear();
> 
> I like your behavior better then Xcode. Xcode seems to dismiss on Escape when non-empty, but here you just clear.

Indeed, I thought this was a better behaviour.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:132
> > +            // FIXME: This should play an error sound.
> 
> You can easily create InspectorFrontendHost.systemBeep. In WebCore the implementation would just call systemBeep() in WebCore/platform/Sound.h.
> 
> As with any InspectorFrontendHost APIs you'd have to update Safari's WebInspector.framework as well. it would be just a few lines following a common pattern. It would call NSBeep().

I thought we already had something like this but couldn't find it. I'll implement it in a separate bug.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:71
> > +    // FIXME: Cmd+L shorcut doesn't actually work.
> > +    new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command, "L", this.showGoToLineDialog.bind(this), this.element);
> 
> I don't have an answer for this right now. If the KeyboardShortcut event listener is setup in the capturing phase, does the web content event listener even get called, or does Safari handle it before us?

I'll have to try it out. Right now the events are always registered in the bubbling phase.

> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:198
> > +        if (!this._goToLineDialog)
> > +            this._goToLineDialog = new WebInspector.GoToLineDialog;
> > +
> > +        this._goToLineDialog.delegate = this;
> 
> Put the delegate assignment inside the lazy instantiation.

Good point.
 
> > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:210
> > +        var range = new WebInspector.TextRange(lineNumber - 1, 0, lineNumber, 0);
> 
> Clever! Does this work for the last line as well (I assume it will, I'd expect CodeMirror to clamp appropriately).

Yes, I tried it and it does, CodeMirror++.

[Antoine Quint]

(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 214362 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214362&action=review
> >
> > > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:71
> > > +    // FIXME: Cmd+L shorcut doesn't actually work.
> > > +    new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command, "L", this.showGoToLineDialog.bind(this), this.element);
> > 
> > I don't have an answer for this right now. If the KeyboardShortcut event listener is setup in the capturing phase, does the web content event listener even get called, or does Safari handle it before us?
> 
> I'll have to try it out. Right now the events are always registered in the bubbling phase.

Hmm, it seems even with a keydown event listener in capture phase registered on the window, Cmd+L is caught by Safari beforehand. Is there any other place in Web Inspector code where we override a default application keyboard shortcut?

[Timothy Hatcher]

Comment on attachment 214362 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:71
>>>> +    new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command, "L", this.showGoToLineDialog.bind(this), this.element);
>>> 
>>> I don't have an answer for this right now. If the KeyboardShortcut event listener is setup in the capturing phase, does the web content event listener even get called, or does Safari handle it before us?
>> 
>> I'll have to try it out. Right now the events are always registered in the bubbling phase.
> 
> Hmm, it seems even with a keydown event listener in capture phase registered on the window, Cmd+L is caught by Safari beforehand. Is there any other place in Web Inspector code where we override a default application keyboard shortcut?

There are some keyboard shortcuts that WebKit can't override, but not because of some explicit blacklist. Command-1..0 are the same way. There is some odd HLToolbox / AppKit bug behind it. I suspect Command-L is one of those.

[Antoine Quint]

System beep is being done in https://bugs.webkit.org/show_bug.cgi?id=122955.

[Antoine Quint]

Created attachment 214445 [details]
Patch

[Timothy Hatcher]

Comment on attachment 214445 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:26
> +WebInspector.GoToLineDialog = function()

I suspect in the future we might want to reuse this or inherit from a base class to share code with Open Quickly.

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:41
> +    this._clearIcon = field.appendChild(document.createElement("img"));
> +    this._clearIcon.src = "Images/CloseWhite.svg";

We try to keep these paths in CSS. There isn't any reason this needs to be in JS.

> Source/WebInspectorUI/UserInterface/GoToLineDialog.js:160
> +WebInspector.GoToLineDialog.prototype.__proto__ = WebInspector.Object.prototype;

We should start putting __proto__ in the prototype object above next to constructor. Brian Burg has started doing this in Timelapse code and it helps readability and ties things together better.

[Antoine Quint]

Created attachment 214505 [details]
Patch for landing

[Antoine Quint]

(In reply to comment #13)
> (From update of attachment 214445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214445&action=review
> 
> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:26
> > +WebInspector.GoToLineDialog = function()
> 
> I suspect in the future we might want to reuse this or inherit from a base class to share code with Open Quickly.

Yes indeed. I'll refactor when that happens.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:41
> > +    this._clearIcon = field.appendChild(document.createElement("img"));
> > +    this._clearIcon.src = "Images/CloseWhite.svg";
> 
> We try to keep these paths in CSS. There isn't any reason this needs to be in JS.

Done.

> > Source/WebInspectorUI/UserInterface/GoToLineDialog.js:160
> > +WebInspector.GoToLineDialog.prototype.__proto__ = WebInspector.Object.prototype;
> 
> We should start putting __proto__ in the prototype object above next to constructor. Brian Burg has started doing this in Timelapse code and it helps readability and ties things together better.

Great, done!

[WebKit Commit Bot]

Comment on attachment 214505 [details]
Patch for landing

Clearing flags on attachment: 214505

Committed r157601: <http://trac.webkit.org/changeset/157601>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.