Skip to content

Replace innerHTML with DOM manipulation in popup.js for Firefox Add-on compliance#182

Merged
rosscado merged 3 commits intomainfrom
copilot/fix-133
Aug 24, 2025
Merged

Replace innerHTML with DOM manipulation in popup.js for Firefox Add-on compliance#182
rosscado merged 3 commits intomainfrom
copilot/fix-133

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Aug 17, 2025

Firefox Add-on submission policies discourage the use of the innerHTML property for security reasons. This PR addresses the remaining innerHTML usage in the popup system by replacing it with proper DOM manipulation methods.

Changes Made

Before:

autoSubmitToggle.innerHTML = `
  <label class="wraper" for="auto-submit">
    <span class="label-text" data-i18n="autoSubmit">Auto-submit</span>
    <div class="switch-wrap control">
      <input type="checkbox" id="auto-submit" name="autoSubmit" />
      <div class="switch"></div>
    </div>
  </label>
`;

After:

const label = document.createElement("label");
label.className = "wraper";
label.setAttribute("for", "auto-submit");

const labelText = document.createElement("span");
labelText.className = "label-text";
labelText.setAttribute("data-i18n", "autoSubmit");
labelText.textContent = "Auto-submit";

const switchWrap = document.createElement("div");
switchWrap.className = "switch-wrap control";

const input = document.createElement("input");
input.type = "checkbox";
input.id = "auto-submit";
input.name = "autoSubmit";

const switchDiv = document.createElement("div");
switchDiv.className = "switch";

switchWrap.appendChild(input);
switchWrap.appendChild(switchDiv);
label.appendChild(labelText);
label.appendChild(switchWrap);
autoSubmitToggle.appendChild(label);

Testing

  • Added comprehensive test coverage in test/popup-dom.spec.ts to verify the DOM structure is created correctly
  • All 235 existing tests continue to pass
  • Build succeeds without errors
  • Verified no remaining innerHTML usage in popup-related files

The refactored code uses Firefox Add-on compliant methods (createElement(), textContent, setAttribute(), appendChild()) while maintaining identical functionality and DOM structure.

Fixes #133.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.saypi.ai
    • Triggering command: node (vitest 3) (dns block)
    • Triggering command: node (vitest 1) (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Aug 17, 2025

@rosscado 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

Co-authored-by: rosscado <16578183+rosscado@users.noreply.github.com>
Copilot AI changed the title [WIP] Status.js should not use innerHtml references Replace innerHTML with DOM manipulation in popup.js for Firefox Add-on compliance Aug 17, 2025
Copilot AI requested a review from rosscado August 17, 2025 16:57
@rosscado rosscado marked this pull request as ready for review August 24, 2025 18:15
@rosscado
Copy link
Copy Markdown
Contributor

@cursoragent can you review this PR?

@cursor
Copy link
Copy Markdown

cursor bot commented Aug 24, 2025

I started by searching the codebase and grepping for innerHTML and auto-submit to understand the existing implementation and the scope of the changes. Then, I read through src/popup/popup.js, test/popup-dom.spec.ts, and popup.html to thoroughly review the refactor.

Based on my review, the PR looks solid and I'm approving it. The refactor successfully replaces innerHTML with safe DOM manipulation methods in src/popup/popup.js, ensuring Firefox Add-on compliance while preserving the original functionality and DOM structure. I verified that:

  • No innerHTML remains in src/popup/*.
  • The created DOM structure matches the previous markup.
  • Internationalization (i18n) is correctly handled with data-i18n attributes and i18nReplace().
  • The toggle is inserted at the correct point in popup.html.
  • The toggle's state wiring (initialization, change handler, and synchronization with the submitMode slider) is correct.

I also confirmed that the new tests in test/popup-dom.spec.ts adequately cover the created DOM structure. I have a couple of non-blocking suggestions, such as initializing the toggle state directly from submitMode to prevent potential drift and considering more robust DOM query assertions in tests, but these do not hinder the PR's approval.


Open in Cursor Open in Web

Learn more about Cursor Agents

@rosscado
Copy link
Copy Markdown
Contributor

initializing the toggle state directly from submitMode to prevent potential drift

Thanks, @cursoragent! Can you please raise a separate issue for that suggestion and assign it to yourself?

@rosscado rosscado merged commit c65dba5 into main Aug 24, 2025
2 checks passed
@rosscado rosscado deleted the copilot/fix-133 branch August 24, 2025 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Status.js should not use innerHtml references

2 participants