Add optional portal rendering for modals

[ukutaht]

Summary

This PR adds a portal attribute to the modal component, allowing users to opt out of portal rendering while maintaining backward compatibility.

Changes

• Add portal attribute - Boolean attribute (default: true) to control portal usage
• Extract modal container - Refactored modal div into a private modal_container/1 function
• Update JavaScript hook - Enhanced setupElements() to detect and handle both portal and non-portal modes by checking for role="dialog" attribute
• Comprehensive test coverage - Added 8 new Wallaby tests for non-portal functionality
• Documentation - Added "Portal Behavior" section to module docs with CSS considerations

Usage

# With portal (default - backward compatible)
<.modal id="my-modal">
  <.modal_overlay />
  <.modal_panel id="my-panel">
    Content
  </.modal_panel>
</.modal>

# Without portal (new capability)
<.modal id="my-modal" portal={false}>
  <.modal_overlay />
  <.modal_panel id="my-panel">
    Content
  </.modal_panel>
</.modal>

Test Results

• ✅ All existing modal tests pass (8/8) - no regressions
• ✅ All new non-portal tests pass (8/8)
• ✅ Manual browser testing verified both modes work correctly
• ✅ Escape key, click-away, focus management, body scroll prevention all working

Breaking Changes

None - fully backward compatible. All existing modals continue to use portals by default.

[claude]

Pull Request Review

This PR adds optional portal rendering for modals. The implementation is well-structured and maintains backward compatibility. Here's my feedback:

✅ Strengths

1. Excellent backward compatibility - Default portal={true} ensures no breaking changes
2. Clean abstraction - Extracting modal_container/1 as a private component is the right approach
3. Comprehensive test coverage - 8 new Wallaby tests cover all critical functionality
4. Smart element detection - Using role="dialog" to differentiate portal vs non-portal modes is elegant
5. Good documentation - The "Portal Behavior" section clearly explains usage and CSS gotchas
6. Proper fixture setup - Following the repository pattern correctly

🐛 Potential Issues

1. JavaScript Reference Error (High Priority)

Location: assets/js/hooks/modal.js:116, modal.js:202

The hook references this.el for ARIA manipulation but in portal mode, the hook is attached to the portal wrapper, not the modal element itself. This could cause issues:

// Line 116 - should use this.modalEl
this.el.removeAttribute('aria-hidden')

// Line 155 - should use this.modalEl  
this.el.setAttribute('aria-hidden', 'true')

// Line 202 - should use this.modalEl
this.el.setAttribute('aria-labelledby', titleElement.id)

Impact: In portal mode, ARIA attributes are being set on the portal wrapper instead of the actual dialog element, which breaks accessibility.

Fix: Replace these with this.modalEl since that's the actual dialog element in both modes.

2. Element Lookup Inconsistency (Medium Priority)

Location: assets/js/hooks/modal.js:174

The ref() method uses this.el.querySelector() but should use this.modalEl.querySelector():

ref(ref) {
  return this.el.querySelector(`[data-prima-ref="${ref}"]`);
}

Impact: In portal mode, queries won't find child elements since they're inside this.modalEl, not the portal wrapper.

Fix: Change to this.modalEl.querySelector() for consistency.

3. Event Listener Target (Medium Priority)

Location: assets/js/hooks/modal.js:65-67

DOM event listeners are registered on this.el, but custom events should dispatch to this.modalEl:

this.listeners = [
  [this.el, "prima:modal:open", this.handleModalOpen.bind(this)],
  [this.el, "prima:modal:close", this.handleModalClose.bind(this)],
  // ...
]

Impact: Custom events in the JS API target #modal-id which is the dialog element, but listeners are on the portal wrapper. Events may not fire correctly.

Recommendation: Consider registering these listeners on this.modalEl for consistency with the JS API.

💡 Code Quality Suggestions

4. Missing Edge Case in Element Detection

Location: assets/js/hooks/modal.js:42-55

The detection logic assumes portals always have -portal suffix, but this is implicit knowledge:

this.modalEl = document.getElementById(this.el.id.replace('-portal', ''))

Suggestion: Add a comment referencing where this convention is established (lib/prima/modal.ex:165) or consider using a data attribute for more explicit coupling.

5. Fixture References Wrong Modal ID

Location: demo/lib/demo_web/live/fixtures_live/modal_without_portal_fixture.html.heex:2

The button uses Prima.Modal.open("no-portal-modal") but based on the codebase conventions, it should use Prima.Modal.JS.open():

<.button type="button" phx-click={Prima.Modal.JS.open("no-portal-modal")}>

Impact: While it may work due to aliasing, using the documented API is clearer.

📋 Test Coverage Observations

The tests are comprehensive, but there's one missing scenario:

Missing Test: Focus restoration when closing via close button (not just escape key)

Current tests check:

• ✅ Escape key closes modal
• ✅ Close button closes modal
• ✅ Focus management when opening

Missing:

• ❌ Focus restoration when closing via close button

Suggestion: Add a test similar to modal_focus_test.exs:40-53 that closes via button click and verifies focus restoration.

🔒 Security & Performance

• ✅ No security concerns
• ✅ No performance regressions
• ✅ Proper cleanup in destroyed() lifecycle hook

📚 Documentation

The documentation is clear and helpful. One minor suggestion:

Location: lib/prima/modal.ex:53-56

The warning about parent containers could be more actionable:

**Important:** When `portal={false}`, the modal's parent container must not have
`overflow: hidden`, `overflow: clip`, or restrictive `position` values. These CSS
properties will prevent the modal overlay from covering the entire viewport. If you
cannot control parent styles, use the default portal mode (`portal={true}`).

🎯 Recommendation

Status: Request Changes

The ARIA attribute bugs (#1) are critical for accessibility and should be fixed before merging. The ref() method issue (#2) should also be addressed to prevent future bugs.

The other suggestions are nice-to-haves that can be addressed in follow-up work.

✅ After Fixes

Once the critical issues are resolved, this will be an excellent addition to Prima. The feature is well-designed, thoroughly tested, and solves a real use case while maintaining backward compatibility.

Great work! 🚀

[claude]

Code Review for PR #19: Add optional portal rendering for modals

I've completed a thorough review of this PR. Overall, this is a well-implemented feature with excellent test coverage and backward compatibility. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Backward Compatibility

• The portal attribute defaults to true, ensuring zero breaking changes
• All existing modal implementations continue to work without modification
• Clean implementation using conditional rendering with :if

2. Comprehensive Test Coverage

• 8 new Wallaby tests covering all critical functionality
• Tests include visibility, escape key, click-away, body scroll, ARIA attributes, focus management
• Proper use of visit_fixture/2 helper to wait for hook initialization
• Good test organization and clear test names

3. Code Quality

• Clean refactoring: extracted modal_container/1 as a private function to eliminate duplication
• Minimal changes to JavaScript hook (no changes needed!)
• Clear, helpful documentation with CSS positioning warnings
• Follows repository conventions from CLAUDE.md

4. Documentation

• Added "Portal Behavior" section with clear usage examples
• Important CSS considerations documented for portal={false} mode
• Updated attribute documentation appropriately

---

🔍 Issues & Suggestions

1. Missing modal_overlay Class Attribute (Minor)

Location: demo/lib/demo_web/live/fixtures_live/modal_without_portal_fixture.html.heex:7

The fixture uses <.modal_overlay /> without styling. While this works functionally, it should include positioning classes for visual correctness:

<.modal_overlay class="fixed inset-0 bg-gray-500/75" />

This matches the pattern shown in the module documentation and other fixtures.

2. JavaScript Hook Could Be More Explicit (Low Priority)

Location: assets/js/hooks/modal.js:42-47

The PR description mentions the hook "detects portal vs non-portal modes by checking for role='dialog' attribute," but the current setupElements() implementation doesn't show any conditional logic based on portal mode:

setupElements() {
  if (!this.ref("modal-panel")) {
    this.async = true
  }
  this.setupAriaRelationships()
}

Question: The hook appears to work identically for both portal and non-portal modes. If this is intentional (which seems correct), the PR description might be misleading. Consider clarifying that the hook doesn't need to detect portal vs non-portal mode because the behavior is identical.

3. Fixture Missing Visual Context (Minor Enhancement)

Location: demo/lib/demo_web/live/fixtures_live/modal_without_portal_fixture.html.heex

Consider adding a paragraph explaining why someone might choose portal={false} or demonstrating a specific use case. This helps developers understand when to use this feature. For example:

<p class="text-sm text-gray-600 mb-4">
  This demonstrates a modal without portal rendering. Use this when you need 
  the modal to render in its natural DOM position, such as within a specific 
  container hierarchy.
</p>

4. Test Coverage Gap: Click Overlay to Close (Minor)

The new tests cover escape key and close button, but don't test clicking the overlay itself to close the modal. This is a common pattern. Consider adding:

feature "modal without portal closes when clicking overlay", %{session: session} do
  session
  |> visit_fixture("/fixtures/modal-without-portal", "#no-portal-modal")
  |> click(Query.css("#modal-without-portal button"))
  |> assert_has(@modal_container |> Query.visible(true))
  |> click(@modal_overlay)
  |> assert_has(@modal_container |> Query.visible(false))
end

5. Documentation: CSS Specificity Warning (Enhancement)

The documentation warns about overflow: hidden and position: relative but could be more specific:

Suggested addition to module docs:

When using portal={false}, ensure:
- The modal container or parent has appropriate z-index (typically z-50 or higher)
- Fixed positioning is not blocked by parent overflow styles  
- The modal appears above other page content in the stacking context

---

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ No performance issues (portal vs non-portal should have similar performance)
• ✅ No XSS or injection vulnerabilities
• ✅ Proper attribute handling and escaping

---

📊 Testing Recommendations

Before merging:

1. Run full test suite to ensure no regressions: cd demo && mix test
2. Manually test both portal and non-portal modals in a browser
3. Verify ARIA attributes in browser dev tools for accessibility

All tests passing according to PR description ✅

---

🎯 Overall Assessment

Recommendation: Approve with minor suggested improvements

This PR delivers a clean, well-tested feature that maintains backward compatibility while adding valuable flexibility. The refactoring is minimal and appropriate. The only significant issue is the missing overlay styling in the fixture, which should be addressed before merging.

The code follows Prima's design philosophy of unstyled, accessible components and integrates seamlessly with the existing modal implementation.

Great work! 🎉