Bug 151865 - Web Inspector: debugger popover should have syntax highlighting when showing a function's source
Summary: Web Inspector: debugger popover should have syntax highlighting when showing ...
Status: RESOLVED DUPLICATE of bug 161417
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: EasyFix, InRadar
Depends on:
Blocks:
 
Reported: 2015-12-04 09:59 PST by BJ Burg
Modified: 2016-09-05 23:06 PDT (History)
7 users (show)

See Also:


Attachments
[Screenshot] unstyled function text (416.17 KB, image/png)
2015-12-04 09:59 PST, BJ Burg
no flags Details
[Patch] Proposed Fix (4.44 KB, patch)
2015-12-10 16:31 PST, Matt Baker
timothy: review-
Details | Formatted Diff | Diff
[Image] New function popover UI (234.15 KB, image/png)
2015-12-10 16:35 PST, Matt Baker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-12-04 09:59:35 PST
Created attachment 266633 [details]
[Screenshot] unstyled function text

It looks like we are just inserting a bunch of function source text into the DOM. It should be a readonly CodeMirror instance with syntax highlighting.
Comment 1 Radar WebKit Bug Importer 2015-12-04 09:59:45 PST
<rdar://problem/23762000>
Comment 2 Matt Baker 2015-12-10 16:31:55 PST
Created attachment 267136 [details]
[Patch] Proposed Fix
Comment 3 Matt Baker 2015-12-10 16:35:04 PST
Created attachment 267137 [details]
[Image] New function popover UI
Comment 4 Timothy Hatcher 2015-12-10 16:46:37 PST
Comment on attachment 267136 [details]
[Patch] Proposed Fix

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

Looks good! The indent of the function body being different from the title bothers me. You should also fix the escape key not working once you click in the function.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:165
> +    padding-top: 4px;

This helps, but these popovers need more work in this area.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1468
> +                readOnly: "nocursor",

Interesting, I didn't know this was an option! It must be new. We had to add this to the style sidebar to hide the cursor, when it seems we could use this now.

.css-style-text-editor.read-only > .CodeMirror .CodeMirror-cursor {
    display: none;
}

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1488
> +            // CodeMirror needs a refresh after the popover displays, to layout, otherwise it doesn't appear.
> +            setTimeout(() => {codeMirror.refresh();}, 0);

I think we should have spaces inside the { }. It would also be best if the Popover class had a hook or event to do this work instead of using a timeout.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1493
> +            // Once CodeMirror renders it's DOM tree, the popover frame size needs to be updated.
> +            codeMirror.on("update", () => {
> +                this._popover.update();
> +            });

Does this lead to a visible resize after it shows?
Comment 5 Timothy Hatcher 2015-12-10 16:49:08 PST
Comment on attachment 267136 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1468
>> +                readOnly: "nocursor",
> 
> Interesting, I didn't know this was an option! It must be new. We had to add this to the style sidebar to hide the cursor, when it seems we could use this now.
> 
> .css-style-text-editor.read-only > .CodeMirror .CodeMirror-cursor {
>     display: none;
> }

Though, hiding the cursor in the way we did for the style sidebar still allows text selection and Command-A, which we want. We want to make sure those work here too. If readOnly: "nocursor" disables those features, we might want to take the CSS approach to hide the cursor instead and do readOnly: true here.
Comment 6 Matt Baker 2015-12-10 17:13:42 PST
(In reply to comment #5)
> Comment on attachment 267136 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267136&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:1468
> >> +                readOnly: "nocursor",
> > 
> > Interesting, I didn't know this was an option! It must be new. We had to add this to the style sidebar to hide the cursor, when it seems we could use this now.
> > 
> > .css-style-text-editor.read-only > .CodeMirror .CodeMirror-cursor {
> >     display: none;
> > }
> 
> Though, hiding the cursor in the way we did for the style sidebar still
> allows text selection and Command-A, which we want. We want to make sure
> those work here too. If readOnly: "nocursor" disables those features, we
> might want to take the CSS approach to hide the cursor instead and do
> readOnly: true here.

Text selection and Command-A work as long as CodeMirror has the focus. We can auto-focus it once the ESC to dismiss issue is resolved.
Comment 7 Devin Rousso 2016-09-05 20:48:01 PDT
This was fixed in r205223.

*** This bug has been marked as a duplicate of bug 161417 ***