Bug 147109 - Web Inspector: Update to CodeMirror 5.5 or later
Summary: Web Inspector: Update to CodeMirror 5.5 or later
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 146115
  Show dependency treegraph
 
Reported: 2015-07-20 07:39 PDT by Timothy Hatcher
Modified: 2015-08-11 22:40 PDT (History)
9 users (show)

See Also:


Attachments
Patch (314.74 KB, patch)
2015-08-11 10:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (325.07 KB, patch)
2015-08-11 16:34 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Animated GIF] Before/after the patch (43.29 KB, image/gif)
2015-08-11 17:18 PDT, Nikita Vasilyev
no flags Details
Patch (326.67 KB, patch)
2015-08-11 21:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2015-07-20 07:39:24 PDT
CodeMirror 5.5 is out. We should update to the latest.

https://twitter.com/codemirror/status/623085362022359040
Comment 1 Radar WebKit Bug Importer 2015-07-20 07:39:32 PDT
<rdar://problem/21898870>
Comment 2 Timothy Hatcher 2015-07-20 07:43:36 PDT
We are using 4.2.0 currently.
Comment 3 Devin Rousso 2015-08-11 10:07:22 PDT
Created attachment 258723 [details]
Patch
Comment 4 Timothy Hatcher 2015-08-11 10:45:30 PDT
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.
Comment 5 Timothy Hatcher 2015-08-11 10:45:49 PDT
Please make sure to test by running OpenSource/Source/WebInspectorUI/Scripts/copy-user-interface-resources-dryrun.rb
Comment 6 Devin Rousso 2015-08-11 13:08:20 PDT
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.
Comment 7 Devin Rousso 2015-08-11 13:16:40 PDT
(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?
Comment 8 Joseph Pecoraro 2015-08-11 13:38:42 PDT
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
Comment 9 Devin Rousso 2015-08-11 16:34:37 PDT
Created attachment 258779 [details]
Patch
Comment 10 Nikita Vasilyev 2015-08-11 17:18:54 PDT
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.
Comment 11 Timothy Hatcher 2015-08-11 19:28:53 PDT
Yes, we need to fix those things. It can happen in a follow up, as long as we don't forget.
Comment 12 Devin Rousso 2015-08-11 21:45:18 PDT
Created attachment 258802 [details]
Patch
Comment 13 WebKit Commit Bot 2015-08-11 22:40:49 PDT
Comment on attachment 258802 [details]
Patch

Clearing flags on attachment: 258802

Committed r188325: <http://trac.webkit.org/changeset/188325>
Comment 14 WebKit Commit Bot 2015-08-11 22:40:52 PDT
All reviewed patches have been landed.  Closing bug.