Bug 122893 - Web Inspector: Go to line keyboard command and dialog
Summary: Web Inspector: Go to line keyboard command and dialog
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-10-16 06:49 PDT by Antoine Quint
Modified: 2013-10-17 14:36 PDT (History)
5 users (show)

See Also:


Attachments
Patch (19.93 KB, patch)
2013-10-16 07:49 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Screenshot of the go-to-line dialog showing over a web page's source code view (78.67 KB, image/png)
2013-10-16 07:55 PDT, Antoine Quint
no flags Details
Patch (19.91 KB, patch)
2013-10-17 05:15 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (19.87 KB, patch)
2013-10-17 14:01 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2013-10-16 06:49:14 PDT
<rdar://problem/14092546>
Comment 1 Antoine Quint 2013-10-16 06:49:44 PDT
There should be a way to easily jump to a line number in a source code editor.
Comment 2 Antoine Quint 2013-10-16 07:49:12 PDT
Created attachment 214362 [details]
Patch
Comment 3 Antoine Quint 2013-10-16 07:50:04 PDT
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?
Comment 4 Antoine Quint 2013-10-16 07:55:30 PDT
Created attachment 214363 [details]
Screenshot of the go-to-line dialog showing over a web page's source code view
Comment 5 Joseph Pecoraro 2013-10-16 10:55:09 PDT
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).
Comment 6 Joseph Pecoraro 2013-10-16 11:02:12 PDT
> > 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.
Comment 7 Timothy Hatcher 2013-10-16 11:04:11 PDT
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.)
Comment 8 Antoine Quint 2013-10-16 13:13:19 PDT
(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++.
Comment 9 Antoine Quint 2013-10-16 13:25:34 PDT
(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?
Comment 10 Timothy Hatcher 2013-10-16 13:37:56 PDT
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.
Comment 11 Antoine Quint 2013-10-17 05:04:03 PDT
System beep is being done in https://bugs.webkit.org/show_bug.cgi?id=122955.
Comment 12 Antoine Quint 2013-10-17 05:15:34 PDT
Created attachment 214445 [details]
Patch
Comment 13 Timothy Hatcher 2013-10-17 09:41:21 PDT
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.
Comment 14 Antoine Quint 2013-10-17 14:01:46 PDT
Created attachment 214505 [details]
Patch for landing
Comment 15 Antoine Quint 2013-10-17 14:03:54 PDT
(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!
Comment 16 WebKit Commit Bot 2013-10-17 14:36:45 PDT
Comment on attachment 214505 [details]
Patch for landing

Clearing flags on attachment: 214505

Committed r157601: <http://trac.webkit.org/changeset/157601>
Comment 17 WebKit Commit Bot 2013-10-17 14:36:47 PDT
All reviewed patches have been landed.  Closing bug.