985924 - Upgrade to CodeMirror 4

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[Brian Grinstead [:bgrins]]

CodeMirror 4 has been released: https://groups.google.com/forum/#!topic/codemirror/5WCakVK_xnY.  Release includes multiple selections, performance improvements, module loader shims.

There is a document with information for upgrading: http://codemirror.net/doc/upgrade_v4.html.

Main points that would affect us for upgrading have to do with multiple selections.  Things like autocomplete need to be aware of multiple selections and do something different (like not show up).  The upgrading document has information that will help - also, searching for "multiple selection" in http://codemirror.net/doc/manual.html gives an idea of all of the APIs that are affected by it.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I'm particularly looking forward to the Sublime Text keybindings. For whoever does the upgrade: if you don't have the time to add them, please file a followup!

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: codemirror-4-WIP.patch

Pulls in latest source, plugins, keymaps including sublime text, and tests.  Code is running, but some stuff is still broken with multi selection (style editor still tries to auto complete when there are multi selections, for instance).

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt1.patch

Updated patch with passing sourceeditor tests

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt2.patch

Follow up patch that fixes autocomplete compat with multiple selections

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8398675 [details] [diff] [review]
CodeMirror-4-pt2.patch

Review of attachment 8398675 [details] [diff] [review]:
-----------------------------------------------------------------

This patch adds compatibility for autocomplete with multiple selections.  Doesn't attempt to autocomplete when there is more than one, but doesn't break either.  Does this look good?  Are there other cases we should cover for autocompletion?

Comment 6

[Girish Sharma [:Optimizer]]

Comment on attachment 8398675 [details] [diff] [review]
CodeMirror-4-pt2.patch

Review of attachment 8398675 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good for update pov.
But I think instead of not auto completing - we can autocomplete like ST2 does i.e. Insert the selected suggestion at each cursor. That would be simply amazing. Will see if I can hack that this weekend .

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> Comment on attachment 8398675 [details] [diff] [review]
> CodeMirror-4-pt2.patch
> 
> Review of attachment 8398675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good for update pov.
> But I think instead of not auto completing - we can autocomplete like ST2
> does i.e. Insert the selected suggestion at each cursor. That would be
> simply amazing. Will see if I can hack that this weekend .

If you apply both patches here then set devtools.editor.keymap to "sublime" you will get the keymap support.  The autocomplete popup actually shows up fine if you just remove my changes to onEditorKeypress, but there are two things that would need to be fixed:
1) When you select one it only sets the text on the first selection.
2) If the first selection is not in the window (above or below the visible area) it jumps back to the top and causes some issues.  I would think that in this case we just don't show the popup.

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> fine if you just remove my changes to onEditorKeypress, but there are two
> things that would need to be fixed:
> 1) When you select one it only sets the text on the first selection.

Yeah exactly ! 

> 2) If the first selection is not in the window (above or below the visible
> area) it jumps back to the top and causes some issues.  I would think that
> in this case we just don't show the popup.

Why not to the one which is visible ? ;)

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #8)

> > 2) If the first selection is not in the window (above or below the visible
> > area) it jumps back to the top and causes some issues.  I would think that
> > in this case we just don't show the popup.
> 
> Why not to the one which is visible ? ;)

This would only make sense if all of the selections have the same autocomplete entries available to them.  Then again, I guess showing the popup at all with multiple selections only makes sense if they all have the same entries available to them.  For instance, given this text I would not expect any autocompletion to happen at all after typing a character that would typically open it up on the first cursor:


    body {
      color: red;
      |
    }|
    |
    he|ad {
      |
    }|

Comment 10

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Girish Sharma [:Optimizer] from comment #8)
> 
> > > 2) If the first selection is not in the window (above or below the visible
> > > area) it jumps back to the top and causes some issues.  I would think that
> > > in this case we just don't show the popup.
> > 
> > Why not to the one which is visible ? ;)
> 
> This would only make sense if all of the selections have the same
> autocomplete entries available to them.  Then again, I guess showing the
> popup at all with multiple selections only makes sense if they all have the
> same entries available to them.  For instance, given this text I would not
> expect any autocompletion to happen at all after typing a character that
> would typically open it up on the first cursor:
> 
> 
>     body {
>       color: red;
>       |
>     }|
>     |
>     he|ad {
>       |
>     }|

Oh boy, yeah, this is tricky. Is there a way of knowing which is the real cursor amongst them ?
Sublime also only suggests if the suggestion list would be same for all the cursors. Thus, it would not suggest for above case.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8398673 [details] [diff] [review]
CodeMirror-4-pt1.patch

Review of attachment 8398673 [details] [diff] [review]:
-----------------------------------------------------------------

Jan, can you please take a look at these 2 patches and let me know if you see any issues with the code or functionality?  The only changes I've made in the devtools code are in Attachment 8398675 [details] [diff], which is a workaround to prevent autocomplete from opening when there is more than one selection.

I've been following the README for updating: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/README?force=1, and I also have a try build: https://tbpl.mozilla.org/?tree=Try&rev=94d3846f493f

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8398673 [details] [diff] [review]
CodeMirror-4-pt1.patch

Review of attachment 8398673 [details] [diff] [review]:
-----------------------------------------------------------------

In general, this is a nice and smooth update! I was expecting the CodeMirror 4 version change to be more complex, but it looks like we dodge most of the already few caveats in http://codemirror.net/doc/upgrade_v4.html

I have a few nits below. When quickly trying your patches, things seemed to mostly work like expected, and multi-select is just awesome! I saw an issue with empty lines having a bigger height than the others on Ubuntu, in both Debugger and Style Editor. I'll take more time to try it out in a subsequent review.

::: browser/devtools/sourceeditor/codemirror/README
@@ +46,1 @@
>  

Nit: Please also update "Currently used version" at line 8 to "4.0".

@@ +79,5 @@
>   * test/cm_mode_test.js
> + * test/cm_multi_test.js
> + * test/cm_search_test.js
> + * test/cm_sublime_test.js
> + * test/cm_test.js

It doesn't look like these new test files are actually integrated with our test harness. Please either leave them out of this patch or integrate them properly.

These look like the proper places to reference them: http://dxr.mozilla.org/mozilla-central/search?q=cm_comment_test.js

@@ +86,4 @@
>  
>  # Localization patches
>  
>  diff --git a/browser/devtools/sourceeditor/codemirror/search/search.js b/browser/devtools/sourceeditor/codemirror/sea

I see that you successfully applied this patch to search.js.

Nit: Please update a rebased version of the patch here.

::: browser/devtools/sourceeditor/test/codemirror.html
@@ +14,5 @@
>      <script src="chrome://browser/content/devtools/codemirror/comment.js"></script>
>      <script src="chrome://browser/content/devtools/codemirror/javascript.js"></script>
>      <script src="chrome://browser/content/devtools/codemirror/vim.js"></script>
>      <script src="chrome://browser/content/devtools/codemirror/emacs.js"></script>
> +    <script src="chrome://browser/content/devtools/codemirror/sublime.js"></script>

Doesn't browser/devtools/sourceeditor/test/vimemacs.html also need to be updated?

Comment 13

[Jan Keromnes [:janx]]

Comment on attachment 8398675 [details] [diff] [review]
CodeMirror-4-pt2.patch

Review of attachment 8398675 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

::: browser/devtools/sourceeditor/autocomplete.js
@@ +161,5 @@
>   */
>  function onEditorKeypress({ ed, Editor }, event) {
>    let private = privates.get(ed);
> +
> +  // Do not try to autocomplete with multiple selections.

This looks like a good enough heuristic for now, but I think eventually it would be great to implement the SublimeText-like autocompletion when all cursors have similar entries.

@@ +163,5 @@
>    let private = privates.get(ed);
> +
> +  // Do not try to autocomplete with multiple selections.
> +  if (ed.hasMultipleSelections()) {
> +      private.doNotAutocomplete = true;

Nit: Please 2-space indents.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Jan Keromnes [:janx] from comment #12)
> Comment on attachment 8398673 [details] [diff] [review]
> CodeMirror-4-pt1.patch
> 
> Review of attachment 8398673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general, this is a nice and smooth update! I was expecting the CodeMirror
> 4 version change to be more complex, but it looks like we dodge most of the
> already few caveats in http://codemirror.net/doc/upgrade_v4.html
> 
> I have a few nits below. When quickly trying your patches, things seemed to
> mostly work like expected, and multi-select is just awesome! I saw an issue
> with empty lines having a bigger height than the others on Ubuntu, in both
> Debugger and Style Editor. I'll take more time to try it out in a subsequent
> review.
> 
> ::: browser/devtools/sourceeditor/codemirror/README
> @@ +46,1 @@
> >  
> 
> Nit: Please also update "Currently used version" at line 8 to "4.0".
> 
> @@ +79,5 @@
> >   * test/cm_mode_test.js
> > + * test/cm_multi_test.js
> > + * test/cm_search_test.js
> > + * test/cm_sublime_test.js
> > + * test/cm_test.js
> 
> It doesn't look like these new test files are actually integrated with our
> test harness. Please either leave them out of this patch or integrate them
> properly.
> 
> These look like the proper places to reference them:
> http://dxr.mozilla.org/mozilla-central/search?q=cm_comment_test.js
> 
> @@ +86,4 @@
> >  
> >  # Localization patches
> >  
> >  diff --git a/browser/devtools/sourceeditor/codemirror/search/search.js b/browser/devtools/sourceeditor/codemirror/sea
> 
> I see that you successfully applied this patch to search.js.
> 
> Nit: Please update a rebased version of the patch here.
> 
> ::: browser/devtools/sourceeditor/test/codemirror.html
> @@ +14,5 @@
> >      <script src="chrome://browser/content/devtools/codemirror/comment.js"></script>
> >      <script src="chrome://browser/content/devtools/codemirror/javascript.js"></script>
> >      <script src="chrome://browser/content/devtools/codemirror/vim.js"></script>
> >      <script src="chrome://browser/content/devtools/codemirror/emacs.js"></script>
> > +    <script src="chrome://browser/content/devtools/codemirror/sublime.js"></script>
> 
> Doesn't browser/devtools/sourceeditor/test/vimemacs.html also need to be
> updated?

Thanks for the feedback.  I've updated the test files to make sure that all of the new tests are being used, and have also updated the README as suggested.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt1.patch

Patch upgrading to sourceeditor to CodeMirror 4.0.3

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt2.patch

Autocomplete compatibility fixes for CodeMirror 4.  Don't show autocomplete when multiple selections are used.

Comment 17

[Brian Grinstead [:bgrins]]

Source editor tests pass locally.  Here is a try push with both patches applied: https://tbpl.mozilla.org/?tree=Try&rev=5229ff0c9ff5

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 8400021 [details] [diff] [review]
CodeMirror-4-pt1.patch

Review of attachment 8400021 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. Thanks for the editor update.

There's one problem:
  https://i.imgur.com/Dn4pzX2.png

Empty lines have a different height that lines with text. Do you know why?

r+ with a grain of salt - this rendering issue is an upstream regression that I would like fixed here or in a follow-up with a fix in this release train.

::: browser/devtools/jar.mn
@@ +44,5 @@
>      content/browser/devtools/codemirror/search.js                      (sourceeditor/codemirror/search/search.js)
>      content/browser/devtools/codemirror/dialog.js                      (sourceeditor/codemirror/dialog/dialog.js)
>      content/browser/devtools/codemirror/dialog.css                     (sourceeditor/codemirror/dialog/dialog.css)
>      content/browser/devtools/codemirror/emacs.js                       (sourceeditor/codemirror/keymap/emacs.js)
> +    content/browser/devtools/codemirror/sublime.js                       (sourceeditor/codemirror/keymap/sublime.js)

nit: the second filename doesnt align with the other lines.

I shrug a little bit at the addition of sublime keybindings. I dont see we need them...

(potential feature-creep making support harder)

::: browser/devtools/sourceeditor/codemirror/README
@@ +84,2 @@
>  
>  # Localization patches

Is the diff below all the patching we do on top of a vanilla codemirror codebase? Or do we make any other changes?

::: browser/devtools/sourceeditor/test/browser.ini
@@ +5,2 @@
>    cm_driver.js
> +  cm_emacs_test.js

cm_emacs_test.js shows twice.

@@ -4,2 @@
>    cm_driver.js
> -  cm_mode_javascript_test.js

Why was this test removed?

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 8400022 [details] [diff] [review]
CodeMirror-4-pt2.patch

Review of attachment 8400022 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one concern, see below.

::: browser/devtools/sourceeditor/autocomplete.js
@@ +103,5 @@
>      // "backgr|" but we need to open the popup at the beginning of the character
>      // "b". Thus we need to calculate the width of the entered part of the token
>      // ("backgr" here). 4 comes from the popup's left padding.
> +
> +    let cursorElement = cm.display.cursorDiv.querySelector(".CodeMirror-cursor");

How does this deal with multiple cursors? cm.display.cursor was a codemirror api feature? Isn't there anything equivalent now?

Slightly worried to see direct access to codemirror's dom.

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #18)
> I shrug a little bit at the addition of sublime keybindings. I dont see we
> need them...
> 
> (potential feature-creep making support harder)

I beg to defer here. Sublime key bindings is a really demanded one and potentially has equal number of users (seekers) as vim/emacs. I personally am looking forward to using them as my main.

Moreover, the ability to choose b/w emacs/vim/sublime give our tools a plus point as it makes the tool into the sweet spot of emacs/vim/sublime users.

Also, having emacs and vim but not sublime would really suck (forgetting the fact that I am personally a sublime user).

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #18)
> Comment on attachment 8400021 [details] [diff] [review]
> CodeMirror-4-pt1.patch
> 
> Review of attachment 8400021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. Thanks for the editor update.
> 
> There's one problem:
>   https://i.imgur.com/Dn4pzX2.png
> 
> Empty lines have a different height that lines with text. Do you know why?

No I don't know why, I know Jan pointed this out too, so I'll take a look into this and figure out what is going on.

> 
> r+ with a grain of salt - this rendering issue is an upstream regression
> that I would like fixed here or in a follow-up with a fix in this release
> train.
> 
> ::: browser/devtools/jar.mn
> @@ +44,5 @@
> >      content/browser/devtools/codemirror/search.js                      (sourceeditor/codemirror/search/search.js)
> >      content/browser/devtools/codemirror/dialog.js                      (sourceeditor/codemirror/dialog/dialog.js)
> >      content/browser/devtools/codemirror/dialog.css                     (sourceeditor/codemirror/dialog/dialog.css)
> >      content/browser/devtools/codemirror/emacs.js                       (sourceeditor/codemirror/keymap/emacs.js)
> > +    content/browser/devtools/codemirror/sublime.js                       (sourceeditor/codemirror/keymap/sublime.js)
> 
> nit: the second filename doesnt align with the other lines.
> 
> I shrug a little bit at the addition of sublime keybindings. I dont see we
> need them...
> 
> (potential feature-creep making support harder)
> 

IMO we should add it - it adds only one source file, has test coverage and it doesn't add any code to our wrapper class.  I have heard requests for it, and as we add editor options into the options panel it will be a nice feature to expose.

> ::: browser/devtools/sourceeditor/codemirror/README
> @@ +84,2 @@
> >  
> >  # Localization patches
> 
> Is the diff below all the patching we do on top of a vanilla codemirror
> codebase? Or do we make any other changes?
> 

It's all the patching we do, besides the other comment in the readme which indicates to comment out a couple of tests: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/README?force=1#28.

> ::: browser/devtools/sourceeditor/test/browser.ini
> @@ +5,2 @@
> >    cm_driver.js
> > +  cm_emacs_test.js
> 
> cm_emacs_test.js shows twice.
> 
> @@ -4,2 @@
> >    cm_driver.js
> > -  cm_mode_javascript_test.js
> 
> Why was this test removed?

I thought it was not part of the repo anymore.  It was this test in v3: https://github.com/marijnh/CodeMirror/blob/v3/mode/javascript/test.js, and I missed that it was also in v4.  I will re-add it back in.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #19)
> Comment on attachment 8400022 [details] [diff] [review]
> CodeMirror-4-pt2.patch
> 
> Review of attachment 8400022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just one concern, see below.
> 
> ::: browser/devtools/sourceeditor/autocomplete.js
> @@ +103,5 @@
> >      // "backgr|" but we need to open the popup at the beginning of the character
> >      // "b". Thus we need to calculate the width of the entered part of the token
> >      // ("backgr" here). 4 comes from the popup's left padding.
> > +
> > +    let cursorElement = cm.display.cursorDiv.querySelector(".CodeMirror-cursor");
> 
> How does this deal with multiple cursors? cm.display.cursor was a codemirror
> api feature? Isn't there anything equivalent now?
> 
> Slightly worried to see direct access to codemirror's dom.

cm.display.cursor is no longer part of the API with the introduction of multiple cursors.  The closes thing I could find now is  cm.display.cursorDiv: https://github.com/marijnh/CodeMirror/blob/master/lib/codemirror.js#L150, which gets its contents replaced by a fragment when the selection is updated: https://github.com/marijnh/CodeMirror/blob/master/lib/codemirror.js#L1258.  We could expose something on the Editor class to hide away this bit of CodeMirror specific logic - but there is also a cm.defaultCharWidth() call right above it that we may want to move also.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #19)
> Comment on attachment 8400022 [details] [diff] [review]
> CodeMirror-4-pt2.patch
> 
> Review of attachment 8400022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just one concern, see below.
> 
> ::: browser/devtools/sourceeditor/autocomplete.js
> @@ +103,5 @@
> >      // "backgr|" but we need to open the popup at the beginning of the character
> >      // "b". Thus we need to calculate the width of the entered part of the token
> >      // ("backgr" here). 4 comes from the popup's left padding.
> > +
> > +    let cursorElement = cm.display.cursorDiv.querySelector(".CodeMirror-cursor");
> 
> How does this deal with multiple cursors? cm.display.cursor was a codemirror
> api feature? Isn't there anything equivalent now?
> 
> Slightly worried to see direct access to codemirror's dom.

And to answer the 'how does this deal with multiple cursors?' question - this function will only run when there is one cursor because of the new ed.hasMultipleSelections() check in onEditorKeyPress.  If we decided to show autocomplete with multiple cursors in the future this would need to be changed.

Comment 24

[Brian Grinstead [:bgrins]]

There is an issue on linux with empty lines as both janx and msucan pointed out: https://i.imgur.com/Dn4pzX2.png.  I've spent some time debugging this, and it is a mysterious problem.

The markup is like this: div > pre > span > span > #text.  The extra height is coming from the pre tag.  It has display:block, line height:18px, and has no top or bottom margins/borders/paddings, but somehow has a height of 28px only when there is no content in the text node below.  Once anything is added (like a space), the height snaps back to 18px with no other changes that I can tell.  I'm thinking this may have to do with it being loaded in a XUL doc, since this is not an issue on the Inspector "Edit as HTML" editor (which is loaded in HTML doc).

Optimizer, I remember you were looking into a bug to always load the editor in an HTML doc, is this still on track to happen?

Comment 25

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> There is an issue on linux with empty lines as both janx and msucan pointed
> out: https://i.imgur.com/Dn4pzX2.png.  I've spent some time debugging this,
> and it is a mysterious problem.
> 
> The markup is like this: div > pre > span > span > #text.  The extra height
> is coming from the pre tag.  It has display:block, line height:18px, and has
> no top or bottom margins/borders/paddings, but somehow has a height of 28px
> only when there is no content in the text node below.  Once anything is
> added (like a space), the height snaps back to 18px with no other changes
> that I can tell.  I'm thinking this may have to do with it being loaded in a
> XUL doc, since this is not an issue on the Inspector "Edit as HTML" editor
> (which is loaded in HTML doc).

FWIW, i can't reproduce on Windows.
 
> Optimizer, I remember you were looking into a bug to always load the editor
> in an HTML doc, is this still on track to happen?

I think so, its really super easy to do. I think I have already suggested the minimal patch for it to you over IRC :)

Comment 26

[Girish Sharma [:Optimizer]]

Comment on attachment 8400022 [details] [diff] [review]
CodeMirror-4-pt2.patch

Review of attachment 8400022 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/sourceeditor/autocomplete.js
@@ +103,5 @@
>      // "backgr|" but we need to open the popup at the beginning of the character
>      // "b". Thus we need to calculate the width of the entered part of the token
>      // ("backgr" here). 4 comes from the popup's left padding.
> +
> +    let cursorElement = cm.display.cursorDiv.querySelector(".CodeMirror-cursor");

Actually, after asking Marijn Haverbeke over mail, he said that although this is not public api, but query selecting the first element with .CodeMirror-cursor should work. We can actually remove the weird cursorDiv or display reference if we want to, querying the wrapper element directly.

Comment 27

[Girish Sharma [:Optimizer]]

(Also, cm.display.cursor was also not public api to begin with, he said)

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt-1.patch

Mihai, can you confirm that this patch fixes the line height issue on empty lines that you were experiencing?  With the new version I'm changing the inline line-height specified in editor.js to a constant number (1.25) instead of 'normal' and this seems to resolve the issue across platforms.

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 8401861 [details] [diff] [review]
CodeMirror-4-pt-1.patch

Thanks for the update.

Now all lines render the same, but they look like they need a slightly higher line-height value. I see it's line-height:1 (no unit, why?). Try a slightly higher value.

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #29)
> Comment on attachment 8401861 [details] [diff] [review]
> CodeMirror-4-pt-1.patch
> 
> Thanks for the update.
> 
> Now all lines render the same, but they look like they need a slightly
> higher line-height value. I see it's line-height:1 (no unit, why?). Try a
> slightly higher value.

Ugh, I had 1.25 locally but must have missed the update in the patch.  I got to 1.25 through trial and error to make it match the current line height with CM3.  I'm  using a unitless line-height so that it will be relative to the font size: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Values.

Comment 31

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt-1.patch

Updated line height to 1.25

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: CodeMirror-4-pt1.patch

Ah file naming fail - had an extra dash in the file name which is why it wasn't pulling in the updated line height

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/14eeb68e5cf2
https://hg.mozilla.org/integration/fx-team/rev/a18a19944652

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14eeb68e5cf2
https://hg.mozilla.org/mozilla-central/rev/a18a19944652