Bug 147109 – Web Inspector: Update to CodeMirror 5.5 or later

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

CodeMirror 5.5 is out. We should update to the latest.

https://twitter.com/codemirror/status/623085362022359040

[Radar WebKit Bug Importer]

<rdar://problem/21898870>

[Timothy Hatcher]

We are using 4.2.0 currently.

[Devin Rousso]

Created attachment 258723 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258723 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:444
> -        if (this._codeMirror.options.readOnly || this._codeMirror.state.focused)
> +        if (this._codeMirror.options.readOnly)

No way to get focused state now?

> Source/WebInspectorUI/UserInterface/Views/HoverMenu.css:62
> +    -webkit-transform: translateY(-1px);

Why? No need to prefix now.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:184
> -    -webkit-transform: translateY(1px);
> +    -webkit-transform: translateY(0.5px);

This is curious. Why? No need to prefix now.

[Timothy Hatcher]

Please make sure to test by running OpenSource/Source/WebInspectorUI/Scripts/copy-user-interface-resources-dryrun.rb

[Devin Rousso]

Comment on attachment 258723 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:444
>> +        if (this._codeMirror.options.readOnly)
> 
> No way to get focused state now?

I think you can still get the focus in the same way, but it seems to fire before this event.  Also, I think that it should still insert newlines even after the editor is focused.

>> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:184
>> +    -webkit-transform: translateY(0.5px);
> 
> This is curious. Why? No need to prefix now.

This was purely a style change.  It appeared slightly offset to me.

[Devin Rousso]

(In reply to comment #5)
> Please make sure to test by running
> OpenSource/Source/WebInspectorUI/Scripts/copy-user-interface-resources-
> dryrun.rb

It's asking for a destination directory.  What exactly does this do/how do I use it?

[Joseph Pecoraro]

Comment on attachment 258723 [details]
Patch

Make sure to test the Pretty Printer as well.
1. Open Source/WebInspectorUI/Tools/PrettyPrinter/index.html
2. Run the tests for each of the different "modes" you should see all PASS messages

[Devin Rousso]

Created attachment 258779 [details]
Patch

[Nikita Vasilyev]

Created attachment 258784 [details]
[Animated GIF] Before/after the patch

There are a couple of issues with the patch:
— Increased line-height.
— vertically misaligned in-code elements, such as go-to arrow icons and type profiler tokens.

I can fix them as a follow up.

[Timothy Hatcher]

Yes, we need to fix those things. It can happen in a follow up, as long as we don't forget.

[Devin Rousso]

Created attachment 258802 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258802 [details]
Patch

Clearing flags on attachment: 258802

Committed r188325: <http://trac.webkit.org/changeset/188325>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.