Refactor: Enterprise-Grade Modal Security, Accessibility, and Perform… - #221
Conversation
…ance Removed innerHTML in favor of secure DOM APIs. Added WCAG 2.1 AA focus trapping and ARIA attributes. Implemented event delegation for better performance. Added comprehensive vitest test suite.
WalkthroughThe share button now uses DOM construction, accessible modal behavior, delegated platform clicks, and configurable labels. Vitest setup and comprehensive tests cover security, accessibility, lifecycle behavior, public API compatibility, and SVG output. The landing page TypeScript configuration also changes its compiler settings. ChangesShare button refactor and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SocialShareButton
participant ModalDOM
participant PlatformContainer
User->>SocialShareButton: open modal
SocialShareButton->>ModalDOM: create and display accessible dialog
SocialShareButton->>ModalDOM: focus first enabled element
User->>ModalDOM: press Tab or Escape
ModalDOM->>SocialShareButton: dispatch keyboard event
SocialShareButton->>ModalDOM: trap focus or close dialog
User->>PlatformContainer: click platform control
PlatformContainer->>SocialShareButton: delegate platform click
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/social-share-button.js`:
- Around line 227-252: Set the social platform buttons created in the platforms
iteration to type="button" immediately after creating each button, ensuring they
do not submit enclosing forms; apply the same attribute to the other newly
created buttons referenced by the consolidated comment.
- Around line 132-134: All dynamically created buttons must explicitly use the
non-submitting button type. In src/social-share-button.js at lines 132-134,
update createButton() to set button.type after creation; make the same change
for closeBtn at lines 174-177, copyBtn at lines 197-199, and each platform btn
in buildPlatformButtons() at lines 231-234.
- Around line 421-423: Update openModal() so previousActiveElement is assigned
only when opening a currently closed modal, preserving the original focused
element across repeated calls. Keep closeModal() restoring focus through the
existing previousActiveElement value.
- Around line 169-199: Externalize the modal strings created in the
modal-building flow around the title, close button, URL input, and copy button.
Add corresponding constructor options alongside the existing buttonText option,
defaulting to “Share”, “Close”, “URL to share”, and “Copy”, then use those
options for the visible text and aria-labels instead of hardcoded values.
- Around line 375-387: Update handleKeydown and closeModal so a pending close
timeout prevents duplicate close handling: when Escape is pressed during the 200
ms close period, do not call closeModal again or emit another close event. Use
the existing close-timeout state as the guard while preserving normal Escape
behavior once no close is pending.
- Around line 114-129: Update _createSVG to stop assigning a hardcoded fill to
the path, allowing the path to inherit the parent SVG’s fill. Preserve the
caller-provided attrs behavior so platform icon creation can pass fill: "white"
and render the glyph with that color.
- Around line 213-226: Move the static `platforms` map out of
`buildPlatformButtons` into a module-level constant, preserving all platform
names, colors, and icon paths so it is allocated once and shared across
instances. Remove the method-local declaration, and add a brief inline comment
immediately before the loop that filters unknown platform keys explaining that
behavior.
- Around line 395-415: Update trapFocus and the modal entrance-focus logic to
use one shared helper for querying focusable elements, excluding disabled
controls. In trapFocus, detect when document.activeElement is outside this.modal
and move focus to the first focusable element while preventing the Tab event;
retain existing first/last wrapping behavior. Replace the duplicated selector
near the entrance-focus call site with the shared helper.
In `@tests/social-share-button.test.js`:
- Around line 346-350: Remove the unused platformCount declaration in
tests/social-share-button.test.js lines 346-350, or use a platforms variable to
assert the rendered button count against platforms.length. Also remove the
unused button variable at lines 424-427, or use it in an assertion confirming
document.body no longer contains it after destroy().
- Around line 227-278: Add a test alongside the existing focus-trap tests that
opens the modal, ensures document.activeElement is outside the modal, dispatches
a bubbling cancelable Tab keydown, and asserts focus is moved into the modal.
Use the existing createInstance and modal setup so trapFocus()’s outside-focus
behavior is covered.
- Around line 71-78: Update the sourceFile construction in the test to resolve
src/social-share-button.js relative to the test module’s import.meta.url rather
than process.cwd(), while preserving the existing file-read and innerHTML
assignment assertion.
- Around line 466-485: Replace the individual public-method existence tests
around createInstance with a single shape check, leaving behavioral coverage to
the method-specific tests. Add behavior coverage for updateOptions(), verifying
that calling updateOptions({ url }) updates the modal input value, using the
existing instance and modal setup helpers.
- Around line 200-230: Replace the real-time Promise delays in the focus and
lifecycle tests around openModal and closeModal with Vitest fake-timer
advancement, enabling fake timers for the test suite and advancing them by the
relevant 10ms or 200ms delays. Add vi.useRealTimers() to the shared afterEach
cleanup so fake timers are restored even when assertions fail.
- Line 26: Update the SocialShareButton assignment in the test setup to use only
mod.default, removing the window.SocialShareButton fallback so broken default
exports fail visibly and the ESLint error is resolved.
- Around line 398-410: Update the “should close modal when clicking the overlay
background” test to trigger the click with instance.modal.click() instead of
constructing and dispatching a MouseEvent or redefining its target. Keep the
existing closeModalSpy assertion unchanged.
- Around line 551-555: Update the test “should set fill='white' on platform icon
SVGs” to query the platform icon’s child path and assert its fill value reflects
the effective white paint, rather than asserting only the parent SVG’s fill
attribute. Use the existing _createSVG behavior as the reference and preserve
the current WhatsApp instance setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70854373-404c-4c99-9fe7-3520fdb0163f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/social-share-button.jstests/social-share-button.test.jsvitest.config.js
| it("should set fill='white' on platform icon SVGs", () => { | ||
| const instance = createInstance({ platforms: ["whatsapp"] }); | ||
| const platformSvg = instance.modal.querySelector(".social-share-platform-icon svg"); | ||
| expect(platformSvg.getAttribute("fill")).toBe("white"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This assertion does not prove the platform icon renders white.
The test checks fill on the <svg> only. _createSVG() also sets fill="currentColor" on the child <path>, and a fill attribute on the path overrides inheritance from the parent <svg>. The icon therefore does not render white, yet this test passes. See the _createSVG() comment in src/social-share-button.js.
Assert the effective paint on the <path>.
💚 Proposed change
it("should set fill='white' on platform icon SVGs", () => {
const instance = createInstance({ platforms: ["whatsapp"] });
const platformSvg = instance.modal.querySelector(".social-share-platform-icon svg");
expect(platformSvg.getAttribute("fill")).toBe("white");
+ // The path must not override the parent fill, otherwise the icon is not white.
+ expect(platformSvg.querySelector("path").getAttribute("fill")).toBeNull();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should set fill='white' on platform icon SVGs", () => { | |
| const instance = createInstance({ platforms: ["whatsapp"] }); | |
| const platformSvg = instance.modal.querySelector(".social-share-platform-icon svg"); | |
| expect(platformSvg.getAttribute("fill")).toBe("white"); | |
| }); | |
| it("should set fill='white' on platform icon SVGs", () => { | |
| const instance = createInstance({ platforms: ["whatsapp"] }); | |
| const platformSvg = instance.modal.querySelector(".social-share-platform-icon svg"); | |
| expect(platformSvg.getAttribute("fill")).toBe("white"); | |
| // The path must not override the parent fill, otherwise the icon is not white. | |
| expect(platformSvg.querySelector("path").getAttribute("fill")).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/social-share-button.test.js` around lines 551 - 555, Update the test
“should set fill='white' on platform icon SVGs” to query the platform icon’s
child path and assert its fill value reflects the effective white paint, rather
than asserting only the parent SVG’s fill attribute. Use the existing _createSVG
behavior as the reference and preserve the current WhatsApp instance setup.
Link your account with GitcordThanks for opening this PR, @maheshMadiwalar18! To receive Discord notifications and contributor tracking for this organization:
Once linked, Gitcord can notify you about reviews, merges, and more. — Posted by Gitcord |
github-actions
Bot
added
no-issue-linked
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/social-share-button.js (1)
383-399: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftHandle keyboard events only for the topmost modal.
Each open instance receives the same
documentkeydown event. If two modals are open, one Escape key callscloseModal()for both instances. Each instance also emits a close event and attempts focus restoration.
SocialShareButton.openModalCountalready supports concurrent modals. Track an active-modal stack, and let only the topmost modal process Escape and Tab events. Remove the instance from that stack during close and destroy. Add a test that opens two instances and verifies that Escape closes only the topmost modal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/social-share-button.js` around lines 383 - 399, Update the keyboard handling around handleKeydown to process Escape and Tab only when the instance is the topmost entry in a shared active-modal stack, using SocialShareButton.openModalCount as the existing concurrent-modal state. Push instances when opening, remove them during closeModal and destroy, and ensure stack cleanup remains correct across all close paths. Add a test that opens two instances and verifies Escape closes only the topmost modal.tests/social-share-button.test.js (1)
279-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the pending-close Escape guard.
This test verifies one Escape event only. It does not verify the branch added for a second Escape during the 200 ms close transition.
Dispatch Escape twice before advancing timers. Assert that
closeModal()or the close analytics event occurs once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/social-share-button.test.js` around lines 279 - 295, Extend the “should close the modal on Escape key” test around createInstance and the Escape dispatches to send a second Escape before advancing the 200 ms close-transition timer, then assert that closeModal or its close analytics event is triggered exactly once while the close is pending.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/social-share-button.js`:
- Line 52: Update the timeout callbacks in both copyLink() and fallbackCopy() to
restore the button text from this.options.copyButtonText instead of the
hardcoded "Copy" value, preserving custom and localized labels after copy
feedback. Add coverage verifying a non-default label remains intact after
successful and failed copy operations.
---
Outside diff comments:
In `@src/social-share-button.js`:
- Around line 383-399: Update the keyboard handling around handleKeydown to
process Escape and Tab only when the instance is the topmost entry in a shared
active-modal stack, using SocialShareButton.openModalCount as the existing
concurrent-modal state. Push instances when opening, remove them during
closeModal and destroy, and ensure stack cleanup remains correct across all
close paths. Add a test that opens two instances and verifies Escape closes only
the topmost modal.
In `@tests/social-share-button.test.js`:
- Around line 279-295: Extend the “should close the modal on Escape key” test
around createInstance and the Escape dispatches to send a second Escape before
advancing the 200 ms close-transition timer, then assert that closeModal or its
close analytics event is triggered exactly once while the close is pending.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 550b4ed1-2ebe-4b45-b42d-8a94b6f10077
⛔ Files ignored due to path filters (1)
landing-page/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/pr-body.mdlanding-page/tsconfig.jsonsrc/social-share-button.jstests/social-share-button.test.js
| modalTitleText: options.modalTitleText || "Share", | ||
| closeButtonText: options.closeButtonText || "Close", | ||
| urlInputAriaLabel: options.urlInputAriaLabel || "URL to share", | ||
| copyButtonText: options.copyButtonText || "Copy", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the configured copy label after feedback.
Line 52 configures copyButtonText, but copyLink() and fallbackCopy() reset the button text to the hardcoded "Copy" value. A localized or customized label is lost after a successful or failed copy operation.
Reset the button with this.options.copyButtonText. Add coverage for a non-default label.
Proposed fix
- copyBtn.textContent = "Copy";
+ copyBtn.textContent = this.options.copyButtonText;Apply the same replacement in both fallbackCopy() timeout callbacks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| copyButtonText: options.copyButtonText || "Copy", | |
| copyBtn.textContent = this.options.copyButtonText; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/social-share-button.js` at line 52, Update the timeout callbacks in both
copyLink() and fallbackCopy() to restore the button text from
this.options.copyButtonText instead of the hardcoded "Copy" value, preserving
custom and localized labels after copy feedback. Add coverage verifying a
non-default label remains intact after successful and failed copy operations.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
1 participant
Hi maintainers! 👋
This is my first contribution to AOSSIE. I have implemented the requested refactor with a focus on security, accessibility, and performance while maintaining backward compatibility.
Changes included:
innerHTMLwith secure DOM APIsI would appreciate any feedback or suggestions for improvement. Thank you for your time and review!
Summary by CodeRabbit
New Features
Bug Fixes
Tests