Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FireMonkey] setting keyMaps using CodeMirror advanced options no longer works #495

Closed
Jontron123 opened this issue Jul 23, 2022 · 15 comments
Labels
addon: FireMonkey bug 🐞 Something isn't working done ✓ Completed

Comments

@Jontron123
Copy link

In version 2.57, setting keyMaps in the CodeMirror section of the advanced settings no longer works. In the browser console I found the following error:

Uncaught TypeError: next is undefined [fm-codemirror.js:7764:11](moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/lib/codemirror/fm-codemirror.js)

I was able to fix this error by changing the the function "getKeyMap" in "fm-codemirror.js" to:

  function getKeyMap(val) {
    if(typeof val == "string" && keyMap[val]){
      return keyMap[val];
    } else {
      return val;
    }
  }

Then there was another error:

‘src’ attribute of <script> element is empty. [options.html:72:1](moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.html)

The keyMap script src was getting updated after the page loaded with the proper javascript location, but the keymap script was still not being loaded. After looking at the changes between v2.56 an v2.57 I found that I was able to edit the file "options.html" and changed:

    <!-- Key Maps -->
    <script id="keyMap" src=""></script>

to:

    <!-- Key Maps -->
    <script id="keyMap"></script>

After making these changes, everything is working correctly again. I don't know if these are the best ways to fix this issue, but this seems to be working fine for me.

@erosman
Copy link
Owner

erosman commented Jul 23, 2022

function getKeyMap(val) ....

This can be done this way as well... but that is something to discuss with CodeMirror

  function getKeyMap(val) {
    return typeof val == "string" && keyMap[val] ? keyMap[val] : val
  }

<script id="keyMap" src=""></script>

This was changed since Mozilla validator complained about inline script when there is no src attribute. I will revert it back for v2.58.

After reverting to <script id="keyMap"></script> would you still get the next is undefined error?

@erosman erosman added bug 🐞 Something isn't working done ✓ Completed labels Jul 23, 2022
@dnordstrom
Copy link

Haha, yeah, I must have started debugging this just as this post was being written... 😄

For others who may be confused right now: if everything seems fine, you've done no major changes, extension storage looks all good, yet CodeMirror just shows an empty editor—this is your issue.

Remove any keyMap property from the CodeMirror options and your anxiety is back to its normal levels pronto! 😛

And apparently it's more or less fixed already, so outstanding work. Much appreciated and makes you realize how dependent on this add-on I've become, hehe. FireMonkey, Tridactyl, and uBlock Origin—trio that can do literally anything and get the web you like. ❤️

@erosman
Copy link
Owner

erosman commented Jul 23, 2022

v2.58 uploaded.... let me know how it goes
There is an HTML tag typo in About.. I have fixed it for the next version

@Jontron123
Copy link
Author

After reverting to <script id="keyMap"></script> would you still get the next is undefined error?

Yes the error still occurs. After some more debugging I have found the problem. The error occurs because the keymap javascript is not running before invoking codemirror. The keymap javascript adds itself to the keyMap object which contains the avaliable keymaps. When getKeyMap is called it returns undefined because the keymap javascript has not been able to add itself to the keyMap object. I set some breakpoints using the debugger and here is order in which things execute:

  1. options.js injects the keymap javascript
  2. options.js calls the CodeMirror.fromTextArea function
  3. The "next is undefined" error occurs
  4. The keymap javascript executes.

I have also tested this by hard coding the keymap javascript into the "options.html" file. When doing this, the keymap javascript is executed before the options.js file and the error does not occur. So to fix this error, somehow the the keymap javascript needs to be executed before the options.js file, or at least before calling the CodeMirror.fromTextArea function. Short of converting the keymaps to modules and importing them, I am not sure how to achieve this.

@erosman
Copy link
Owner

erosman commented Jul 24, 2022

Thank you for testing it.

It was working before with the same code, so it seems that changes are might be related to changes in Firefox.
Can you also check to make sure that vim.js etc is loaded and there is no loading issues?

My guess is that the async loading process takes too long.
The Options Page process is:

  • options.js starts
  • pref is loaded from storage (line 1453)
  • scripts are prepared (line 1455)
  • keyMap is loaded (if any) (line 661)
    At this time CodeMirror is not started yet, it will only start when a script is due to be shown in the the editor

Solutions:

  • Add a process to wait for the keyMap to be loaded (if any)
  • Auto-load all keyMaps regardless
    • Pro: less code in options.js
    • Con: 267kb JS will be loaded which may not be needed or even if needed, only one would be needed

PS. I cant produce the error when loading vim.js (but I am loading FM unpacked). Which keyboard shortcuts are you using? (I dont use vim)

Also note:

 Firefox Ubunutu: Editor does not intercept Ctrl-Q 
 https://github.com/codemirror/codemirror5/issues/6958

@Jontron123
Copy link
Author

I am using the sublime keymap, but the vim keymap gives me the same "next is undefined" error. If I hard coding the vim keymap javascript into the "options.html" file I do not get any errors.

When the keyMap is loaded (line 661), The keymap script doesn't execute immediately. It doesn't actually run until after "CodeMirror.fromTextArea" (line 306, the call that causes the error) and "pref.customOptionsCSS" (line 1460) (which I believe signals the end of the initialization and the end of options.js correct?)

Here is the whole call stack for the error if that helps:

Uncaught TypeError: next is undefined
    defineOptions moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/lib/codemirror/fm-codemirror.js:7764
    CodeMirror moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/lib/codemirror/fm-codemirror.js:7928
    CodeMirror moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/lib/codemirror/fm-codemirror.js:7872
    fromTextArea moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/lib/codemirror/fm-codemirror.js:9735
    setCodeMirror moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.js:306
    showScript moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.js:751
    addScript moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.js:683
    process moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.js:1438
    <anonymous> moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.js:1457
    promise callback* moz-extension://d7caabfe-f9ab-4284-bc29-f880bbff49fc/content/options.js:1453
fm-codemirror.js:7764:11

I was able to get it working by using import and await in place of adding a src to the script tag. Here are the changes I made:

Replace adding src attribute with import (line 662):

    cmOptions.keyMap &&
      // (document.querySelector('#keyMap').src = `../lib/codemirror/keymap/${cmOptions.keyMap}.js`);
      await import(`../lib/codemirror/keymap/${cmOptions.keyMap}.js`);

Make the process() function of the script class async to allow the use of await (line 658):

  async process() {
    // --- CodeMirror Key Maps

Make the Arrow Function for "getPref().then" async and add await to "script.process()" because "nav.process()" fails if run before "script.process()" finishes (line 1453).

// ----------------- User Preference -----------------------
App.getPref().then(async () => {
  options.process();
  await script.process();
  showLog.process();
  nav.process();

Again this may not be the best way to do this (I am by no means a professional coder). This kinda goes along with your first solution of waiting for the keyMap to be loaded. I could not find any other way to load an external javascript file, execute it immediately and wait for it to finish.

@erosman
Copy link
Owner

erosman commented Jul 24, 2022

Thank you for testing.

Can you test with FM v2.56?
I would like to know if the issue has come about because of changes in v2.57-2.58 or due to changes in Firefox?

Which Firefox are you testing it on and which OS?

re: await import(`../lib/codemirror/keymap/${cmOptions.keyMap}.js`);
Above import shouldn't work in extension environment since emacs|sublime|vim are not export modules. That type of import works in node.js. Are you compiling the addon before testing?

@Jontron123
Copy link
Author

Can you test with FM v2.56?

I wet back and tested old versions, and the "next is undefined" error happens in every version since v2.43 when it was implemented.

Which Firefox are you testing it on and which OS?

I am using Windows 7 and am testing on both Firefox esr v91.11.0 as well as the latest v102.0.1.

Are you compiling the addon before testing?

I am just editing individual files and replacing them in the .xpi.

import shouldn't work in extension environment since emacs|sublime|vim are not export modules.

Ya, that is why I mentioned that might not be the best way. I could not find any other way to make it work without major changes to the code.

@erosman
Copy link
Owner

erosman commented Jul 24, 2022

I wet back and tested old versions, and the "next is undefined" error happens in every version since v2.43 when it was implemented.

Therefore, the error has come about due to changes to the Firefox since AFAIK it was working before.
Out of curiosity, does it happen on every keyboard combination, or only a few?
I want to make sure it is not related to the keyMap file itself.

I will work on a solution.

@Jontron123
Copy link
Author

Yes, the error happens with all of the key map files. It is looking like the best way to fix this is to do like you pointed out with Stylus and just load them all at once. No messy code to deal with.

I ran into another issue that caused the error also. When changing keymap value in the advanced config, the same error would appear after hitting the save button. The error occurs because the the javascript for the new keymap has not been run. If all the keymaps are loaded at once, this issue would be avoided too.

@erosman
Copy link
Owner

erosman commented Jul 25, 2022

I was also able to reproduce the error when moving from toolbar popup -> script -> Info -> Edit
Above process goes directly to the editor and it runs while the keyMaps are loading.

If I go to Options and then click on a script to open an editor, the error doesn't show for me.

@erosman
Copy link
Owner

erosman commented Jul 25, 2022

v2.59
Updated CodeMirror keyMap process
They will be loaded automatically in option page.

@erosman
Copy link
Owner

erosman commented Jul 25, 2022

v2.59 uploaded.... let me know how it goes

@Jontron123
Copy link
Author

v2.59 works great! No errors and all the key maps work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: FireMonkey bug 🐞 Something isn't working done ✓ Completed
Projects
None yet
Development

No branches or pull requests

3 participants