MrOtherGuy / fx-autoconfig

Load custom javascript in browser context
Mozilla Public License 2.0
173 stars 11 forks source link

Fix malformed XUL exception #14

Closed aminomancer closed 2 years ago

aminomancer commented 2 years ago

Since the userScripts menu is built with parseXULToFragment now, there's an unhandled error when loading a script whose name or filename contains an XML special character, which prevents the menu from being built. Resolved by replacing the characters with their escaped varieties. Sorry, github automatically adds a newline for some reason

aminomancer commented 2 years ago

By the way, you could possibly localize the restart item's label in that menu. This seems to work:

if(isWindow && menu){
  window.MozXULElement.insertFTLIfNeeded("browser/preferences/preferences.ftl");
  function escapeXUL(markup) {
    return markup.replace(/[<>&'"]/g, (char) => {
      switch (char) {
        case `<`:
          return "&lt;";
        case `>`:
          return "&gt;";
        case `&`:
          return "&amp;";
        case `'`:
          return "&apos;";
        case '"':
          return "&quot;";
      }
    });
  }
  let menuFragment = window.MozXULElement.parseXULToFragment(`
    <menu id="userScriptsMenu" label="userScripts">
      <menupopup id="menuUserScriptsPopup" onpopupshown="_ucUtils.updateMenuStatus(this)">
        <menuitem id="userScriptsRestart" oncommand="_ucUtils.restart(true)" tooltiptext="Toggling scripts requires restart"></menuitem>
        <menuseparator></menuseparator>
      </menupopup>
    </menu>
  `);
  const itemsFragment = window.MozXULElement.parseXULToFragment("");
  for(let script of this.scripts){
    itemsFragment.append(
      window.MozXULElement.parseXULToFragment(`
        <menuitem type="checkbox"
                  label="${escapeXUL(script.name || script.filename)}"
                  filename="${escapeXUL(script.filename)}"
                  checked="true"
                  oncommand="_ucUtils.toggleScript(this)">
        </menuitem>
    `)
    );
  }
  menuFragment.getElementById("menuUserScriptsPopup").appendChild(itemsFragment);
  menu.parentNode.insertBefore(menuFragment,menu);
  document.l10n
    .formatValue("should-restart-title")
    .then((c) =>
      document.getElementById("userScriptsRestart").setAttribute("label", c)
    );
}
MrOtherGuy commented 2 years ago

Good catch. This certainly needs to be fixed and your patch looks good.

Thanks!

MrOtherGuy commented 2 years ago

Also, using localization sounds good, although I think I'll keep the existing order where the restart item is at bottom.

Come to think of it, I think I would move the ecapeXUL function to the module global scope because I can see a possibility where we might want to use it elsewhere too.

aminomancer commented 2 years ago

Also, using localization sounds good, although I think I'll keep the existing order where the restart item is at bottom.

Come to think of it, I think I would move the ecapeXUL function to the module global scope because I can see a possibility where we might want to use it elsewhere too.

Oh whoops, I forgot I changed that. I have too many scripts that it bleeds off the screen 😆