1132557 - Upgrade to CodeMirror 5.7.0

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P1)

Description

[Ian Moody [:Kwan] (UTC+1)]

Attachment: rough python upgrade script

So once CodeMirror 4.13 is out it'd be nice to upgrade to it since I've added support for CSS variables (which currently break parsing slightly), and fixed @supports and @-moz-document blocks so they highlight properly (bug 977665)

I have a rough python script I'm attaching that handles most of the legwork for me (actually the first time I've written any python, so I apologise in advance.  I think adding error handling to try and make using it friendlier made the code rather less friendly).  I figure it might be nice to have on record/possibly reuseable in future.

Currently the main issue is changes introduced in 4.7 break a number of tests
TEST-UNEXPECTED-FAIL | Tern is (currently) defined on the window
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_autocomplete_basic.js#31
#36, #51
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_autocomplete_js.js#26

TEST-UNEXPECTED-FAIL | (gutters|folderGutters) is correct
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_prefs.js#63
#81, #92

I think there are also some failures of the CodeMirrors tests that I can/have fixed by updating imports, but I can worry about that more once these main issues are fixed.

Comment 1

[Brian Grinstead [:bgrins]]

> Currently the main issue is changes introduced in 4.7 break a number of tests
> TEST-UNEXPECTED-FAIL | Tern is (currently) defined on the window
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> test/browser_editor_autocomplete_basic.js#31
> #36, #51
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> test/browser_editor_autocomplete_js.js#26
>
> TEST-UNEXPECTED-FAIL | (gutters|folderGutters) is correct
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> test/browser_editor_prefs.js#63
> #81, #92
> 

Just to narrow down the issue to see if it is test-only or if functionality is broken after the upgrade:
1) Does autocompletion work for JS (in scratchpad) with the new version applied?  The easiest way to check is to press ctrl+space and see if you see a listing show up of normal window variables.
2) Do you see gutters for code folding in the style editor?

If either of these aren't working, are you seeing any errors in the browser console when the editor opens up?

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

Attachment: workingOk.png

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Just to narrow down the issue to see if it is test-only or if functionality
> is broken after the upgrade:
> 1) Does autocompletion work for JS (in scratchpad) with the new version
> applied?  The easiest way to check is to press ctrl+space and see if you see
> a listing show up of normal window variables.
> 2) Do you see gutters for code folding in the style editor?
> 
> If either of these aren't working, are you seeing any errors in the browser
> console when the editor opens up?
Well what do you know, both of those are working fine.  And not an error in the console in sight.  That was with 4.7, since that's the upgrade that was at the top of my patch queue, but I'll double check master works okay as well...  yep, exactly the same results.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Ian Moody [:Kwan] from comment #2)
> Created attachment 8564380 [details]
> workingOk.png
> 
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > Just to narrow down the issue to see if it is test-only or if functionality
> > is broken after the upgrade:
> > 1) Does autocompletion work for JS (in scratchpad) with the new version
> > applied?  The easiest way to check is to press ctrl+space and see if you see
> > a listing show up of normal window variables.
> > 2) Do you see gutters for code folding in the style editor?
> > 
> > If either of these aren't working, are you seeing any errors in the browser
> > console when the editor opens up?
> Well what do you know, both of those are working fine.  And not an error in
> the console in sight.  That was with 4.7, since that's the upgrade that was
> at the top of my patch queue, but I'll double check master works okay as
> well...  yep, exactly the same results.

So the functionality is working but the tests aren't?  Or are the tests also working now with the latest patch?

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> So the functionality is working but the tests aren't?
Correct.

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

I assume we'll want to update to 5.0 rather than 4.13, since 5 seems to have the changes you want for bug 1088804.

I cannot for the life of me figure out why browser_editor_autocomplete_js.js is failing.  Though I have noted that even hopping into the test editor window and manually hitting ctrl+space does nothing, whereas it does in Scratchpad, so maybe the test editor just isn't being setup properly?

And what should be done about the win.tern tests?  Just toggling them to !win.tern is obviously enough to get them to pass, but then they seem a bit pointless.

Manually checked still working: 
Scratchpad autocompletion using ctrl+space
Style Editor autocompletion with auto-popup + enter

Note to self: vim tests needs Pos = CodeMirror.Pos setting, and probably import adjustments, to pass.

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

I've also tested cold folding in both the Style Editor and Scratchpad, and it works in both places.

Things of note:

Pre-upgrade:
The in-tree codemirror/README claims that "In cm_comment_test.js [...] fallbackToBlock and fallbackToLine tests" must be commented out.  This is not currently the case.  Why don't they fail then?  Because this line
https://hg.mozilla.org/mozilla-central/file/0cefb584fd1a/browser/devtools/sourceeditor/test/codemirror.html#l65
means they never run, nor do any of the other tests before the 2nd driver.js import. Removing that import, they now do run, and need to be commented out.
In addition, running all tests by removing the 2nd driver import reveals that doc_docKeepsMode also currently fails (since it relies on the markdown mode existing) and must be commented out.
cm_mode_javascript_test.js never runs because it isn't in test/browser.ini
CSS and XML modes tests are currently in the "editor doesn't use these modes" list, even though it does.

Post-upgrade:
core_collapsedRangeCoordsChar, core_wrappingAndResizing, core_moveVstuck all fail, I don't know why, and need to be commented out.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Ian Moody [:Kwan] from comment #5)
> I assume we'll want to update to 5.0 rather than 4.13, since 5 seems to have
> the changes you want for bug 1088804.

I wouldn't want to do the work to upgrade to 4.13 if we are going to very quickly switch to 5, so let's check about the relative stability of 5.0.  Marijn, how do you feel about Dev Tools upgrading to the mobile edition, in terms of stability and partformance?  I notice it's listed at 'experimental' in the release notes.

Comment 8

[Brian Grinstead [:bgrins]]

> I cannot for the life of me figure out why browser_editor_autocomplete_js.js
> is failing.  Though I have noted that even hopping into the test editor
> window and manually hitting ctrl+space does nothing, whereas it does in
> Scratchpad, so maybe the test editor just isn't being setup properly?
> 
> And what should be done about the win.tern tests?  Just toggling them to
> !win.tern is obviously enough to get them to pass, but then they seem a bit
> pointless.

Can you attach a patch with your current work?  I will see if there is anything sticking out.  Feel free to re-enable any tests that you mention in Comment 6, but please don't comment out any failing ones yet.

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Part 1 - Upgrade CodeMirror files to 5.0

copies the new files over

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Part 2 - Apply Mozilla customisations

Perform the Mozilla-specific patching, as mentioned in the README.  Includes the previous missing disabling of the fallback tests, and disabling the docKeepsMode test.

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Part 3 - update the test support-files so that the mode tests run

Comment 12

[Marijn Haverbeke]

> Marijn, how do you feel about Dev Tools upgrading to the mobile edition, in terms of stability and partformance?  I notice it's listed at 'experimental' in the release notes.

The old textarea-based input implementation should be stable, so as long as you are on a desktop browser and don't explicitly enable the contenteditable-based one, you are unlikely to run into any problems.

However, since I get the impression screen reader support is important for Mozilla, you might want to consider turning on the contenteditable implementation, to test it and help me get it to a point where we can consider it stable. That's a call you have to make.

Comment 13

[Brian Grinstead [:bgrins]]

Alright, let's proceed by upgrading to 5.0 without turning on contentEditable in this bug, and then do further testing to decide whether we should enable that in Bug 1088804

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8568896 [details] [diff] [review]
Part 2 - Apply Mozilla customisations

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

> Perform the Mozilla-specific patching, as mentioned in the README.  Includes the previous missing disabling of the fallback tests, and disabling the docKeepsMode test.

Can you please update the README with any relevant info?

Comment 15

[Brian Grinstead [:bgrins]]

Try push with the patch queue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87f3d993d310

Comment 16

[arni2033]

Please, mark this bug as blocking for bug 1176964

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Upgrade existing addons to CodeMirror 5.6.0 [1.0]

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Upgrade existing CodeMirror keymap to 5.6.0 [1.0]

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Update CodeMirror License [1.0]

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Upgrade existing CodeMirror mode to 5.6.0 [1.0]

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Upgrade to CodeMirror 5.6.0 [1.0]

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Update CodeMirror License

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Upgrade existing addons to CodeMirror 5.6.0

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Apply CodeMirror customization for search.js

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Upgrade existing CodeMirror keymap to 5.6.0

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Upgrade existing CodeMirror mode to 5.6.0

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Upgrade to CodeMirror 5.6.0

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Upgrade existing CodeMirror tests to 5.6.0

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Upgrade existing CodeMirror tests to 5.6.0

Comment 31

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Editor config option should be updated

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=533a94de2660

Comment 33

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Editor should emit a cursorActivity on setCursor

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Make Debugger breadrumbs keyboard access test by refocusing the stack frame

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 11: Scratchpad should use capture on the paste event

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 12: Stylesheet Editor should always batch up updates

Comment 37

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c7bcf3159bf

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 13: Enable CodeMirror test for javascript modes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a7c9e69efbe

Comment 39

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 14: Update CodeMirror README for version 5.6.0

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5b697206a16

Comment 41

[Brian Grinstead [:bgrins]]

Gabriel, all of these patches are going to need to be rebased on top of Bug 912121 (assuming that sticks on fx-team), sorry :(.  I can still do the reviews in the meantime.   I think you could find/replace on the patch files to do something like:

browser/devtools/ -> devtools/client/
toolkit/devtools/server -> devtools/server
toolkit/devtools -> devtools/shared
browser/themes/shared/devtools -> devtools/client/themes

And that should get you most of the way.  Also see https://github.com/jryans/devtools-migrate#updating-your-patches

Comment 42

[Brian Grinstead [:bgrins]]

I made a simple script that can migrate the file paths on a patch file: https://gist.github.com/bgrins/8be6ad65a2fd41602f18

Comment 43

[Brian Grinstead [:bgrins]]

From what I understand we are going to land 1206425 before this.  Is that right Gabriel?  I'm going to clear reviews for now since these need rebasing anyway, and the patches will change quite a bit after the reorg.  We may also want to bump to 5.7 (up to you if you want to do that now or in another bug)

Comment 44

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Update CodeMirror License [1.0]

Comment 45

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Upgrade existing addons to CodeMirror 5.7.0 [1.0]

Comment 46

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Apply CodeMirror customization for search.js [1.0]

Comment 47

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Upgrade existing CodeMirror keymap to 5.7.0 [1.0]

Comment 48

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Upgrade existing CodeMirror mode to 5.7.0 [1.0]

Comment 49

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Upgrade to CodeMirror 5.7.0 [1.0]

Comment 50

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Upgrade existing CodeMirror tests to 5.7.0 [1.0]

Comment 51

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Editor config option should be updated [1.0]

Comment 52

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Editor should emit a cursorActivity on setCursor [1.0]

Comment 53

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Make Debugger breadrumbs keyboard access test by refocusing the stack frame [1.0]

Comment 54

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 11: Scratchpad should use capture on the paste event [1.0]

Comment 55

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 12: Stylesheet Editor should always batch up updates [1.0]

Comment 56

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Upgrade existing CodeMirror tests to 5.7.0 [2.0]

Comment 57

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 13: Update CodeMirror README for version 5.7.0 [1.0]

Comment 58

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c6a4ddd7189

Comment 59

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/try/pushloghtml?changeset=94106a77787a

Comment 60

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94106a77787a

Comment 61

[Brian Grinstead [:bgrins]]

Comment on attachment 8665887 [details] [diff] [review]
Part 12: Stylesheet Editor should always batch up updates [1.0]

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

Tom, just a heads up that we are planning to remove the 'immediate' functionality inside updateStyleSheet on the frontend.  We couldn't find anywhere this was intentionally being called, and the event object was being passed in as the first parameter so the truthy check on line 482 was incorrectly being hit.  Seem alright with you?

Comment 62

[Brian Grinstead [:bgrins]]

Gabe, a note about landing this: let's fold all the upgrade stuff into one patch.  Believe that's Parts 1, 2, 4, 5, 6, 7, and 13.  Then the various fixes needed for the upgrade could fold into a second patch - 3, 8, 9, 10, 11, and 12.

Comment 63

[Brian Grinstead [:bgrins]]

Comment on attachment 8665878 [details] [diff] [review]
Part 8: Editor config option should be updated [1.0]

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

::: devtools/client/sourceeditor/editor.js
@@ +1023,5 @@
>        this.config.autocomplete = v;
>        this.setupAutoCompletion();
>      } else {
>        cm.setOption(o, v);
> +      this.config[o] = v;

I can't remember why we needed to do this.  Do you?  It does make sense that we'd keep the config state up to date though (as we do with config.autocomplete above)

Comment 64

[Brian Grinstead [:bgrins]]

ni? for Comment 63

Comment 65

[Tom Tromey :tromey]

Comment on attachment 8665887 [details] [diff] [review]
Part 12: Stylesheet Editor should always batch up updates [1.0]

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

Looks good; and fwiw won't cause any problems with the authored series.

Comment 66

[Brian Grinstead [:bgrins]]

Comment on attachment 8665879 [details] [diff] [review]
Part 9: Editor should emit a cursorActivity on setCursor [1.0]

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

Ship it!

Comment 67

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Brian Grinstead [:bgrins] from comment #63)
> Comment on attachment 8665878 [details] [diff] [review]
> Part 8: Editor config option should be updated [1.0]
> 
> Review of attachment 8665878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/sourceeditor/editor.js
> @@ +1023,5 @@
> >        this.config.autocomplete = v;
> >        this.setupAutoCompletion();
> >      } else {
> >        cm.setOption(o, v);
> > +      this.config[o] = v;
> 
> I can't remember why we needed to do this.  Do you?  It does make sense that
> we'd keep the config state up to date though (as we do with
> config.autocomplete above)

I believe this was because the autocomplete test was failing since the config is not actually saved.

Comment 68

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/0390e740bfc55c2e99cfed3654c1f888e5da95e0
Bug 1132557 - Part 1: Upgrade to CodeMirror 5.7.0 r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/2bfe5435c1d1e2784af3b225d629a210023daecd
Bug 1132557 - Part 2: Apply fixes for CodeMirror 5.7.0 upgrade r=bgrins

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0390e740bfc5
https://hg.mozilla.org/mozilla-central/rev/2bfe5435c1d1