Bug 151865 – Web Inspector: debugger popover should have syntax highlighting when showing a function's source

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Web Inspector

[Blaze Burg]

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.

[Radar WebKit Bug Importer]

<rdar://problem/23762000>

[Matt Baker]

Created attachment 267136 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 267137 [details]
[Image] New function popover UI

[Timothy Hatcher]

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?

[Timothy Hatcher]

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.

[Matt Baker]

(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.

[Devin Rousso]

This was fixed in r205223.

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