Frontend/safari fixes#995
Conversation
WalkthroughAdds device/browser detection to adjust navigation rendering and styles (including iPhone Safari and touch devices), refines media queries to target hover-capable tablets, adds keyboard focus-trap handling for the payment iframe, and tweaks modal and layout styles for responsiveness. Changes
Sequence Diagram(s)Payment popup keyboard focus trap flowsequenceDiagram
participant User
participant Browser
participant PrivilegePurchaseAcceptUI
User->>Browser: Open payment flow
Browser->>PrivilegePurchaseAcceptUI: Render iframe + container
PrivilegePurchaseAcceptUI->>PrivilegePurchaseAcceptUI: create trap element, set title, focus container, attach keydown handlers
User->>PrivilegePurchaseAcceptUI: press Tab / Shift+Tab
PrivilegePurchaseAcceptUI->>PrivilegePurchaseAcceptUI: focusTrap() cycles focus between container and trapper
PrivilegePurchaseAcceptUI->>PrivilegePurchaseAcceptUI: detectIframeClose() -> handleIframeClosed() -> cleanup
Navigation detection and style application (high level)sequenceDiagram
participant Browser
participant PageMainNav
participant CSS
Browser->>PageMainNav: instantiate component
PageMainNav->>PageMainNav: compute isTouchDevice, isIphoneSafari, isMenuTouchToggle
PageMainNav->>PageMainNav: add conditional classes to nav (`touch-device`, `iphone-safari`) and adjust handlers
PageMainNav->>CSS: CSS rules apply based on classes & refined media queries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts (1)
141-178: Focus trap setup looks good with a minor suggestion.The implementation correctly sets up focus trap elements and event listeners. The accessibility improvement with the iframe title is also good.
Consider using
requestAnimationFrameornextTickinstead ofsetTimeoutfor more deterministic focus timing:- window.setTimeout(() => { acceptUiContainer.focus(); }, 100); + this.$nextTick(() => { acceptUiContainer.focus(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
webroot/src/components/Page/PageMainNav/PageMainNav.less(1 hunks)webroot/src/components/Page/PageMainNav/PageMainNav.ts(1 hunks)webroot/src/components/Page/PageMainNav/PageMainNav.vue(1 hunks)webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts(3 hunks)webroot/src/components/UserAccount/UserAccount.less(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: jsandoval81
PR: csg-org/CompactConnect#799
File: webroot/src/pages/LicenseeProof/LicenseeProof.less:144-168
Timestamp: 2025-05-07T18:15:17.462Z
Learning: In the CompactConnect project, print layouts for footers are designed to use fixed positioning (position: fixed) in Chrome and Firefox while using a fallback to relative positioning for Safari (detected via supports (font: -apple-system-body)). This approach is intentional and aligns with the design requirements, as Safari doesn't handle fixed elements correctly in print layouts.
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.745Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (`isDisabled: this.isStaff`) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.
Learnt from: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/styles.common/_inputs.less:115-118
Timestamp: 2025-06-24T00:17:31.188Z
Learning: The team intentionally uses broad CSS selectors like `.dp__input_wrap div { position: static; }` to fix styling issues with the Vue Date-Picker library. They have experience with these styles working well, keep the library version pinned in yarn.lock, and have established processes to re-test everything when updating the library version. This approach is acceptable given their testing discipline and version control.
📚 Learning: in the useraccount component (webroot/src/components/useraccount/useraccount.ts), the email field sh...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.745Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (`isDisabled: this.isStaff`) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.
Applied to files:
webroot/src/components/UserAccount/UserAccount.less
📚 Learning: the `.add-email-help` element in the inputemaillist component (webroot/src/components/forms/inputema...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#822
File: webroot/src/components/Forms/InputEmailList/InputEmailList.less:17-26
Timestamp: 2025-05-28T16:10:55.628Z
Learning: The `.add-email-help` element in the InputEmailList component (webroot/src/components/Forms/InputEmailList/InputEmailList.less) is display-only and not meant to handle interaction states like hover or focus. It should not have cursor: pointer or interactive styling.
Applied to files:
webroot/src/components/UserAccount/UserAccount.less
📚 Learning: in the compactconnect project, print layouts for footers are designed to use fixed positioning (posi...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#799
File: webroot/src/pages/LicenseeProof/LicenseeProof.less:144-168
Timestamp: 2025-05-07T18:15:17.462Z
Learning: In the CompactConnect project, print layouts for footers are designed to use fixed positioning (position: fixed) in Chrome and Firefox while using a fallback to relative positioning for Safari (detected via supports (font: -apple-system-body)). This approach is intentional and aligns with the design requirements, as Safari doesn't handle fixed elements correctly in print layouts.
Applied to files:
webroot/src/components/Page/PageMainNav/PageMainNav.less
📚 Learning: in privilegepurchaseacceptui, the team requires that the acceptui script be appended only to the ele...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#847
File: webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts:91-109
Timestamp: 2025-06-11T18:47:17.329Z
Learning: In PrivilegePurchaseAcceptUI, the team requires that the AcceptUI script be appended only to the element with id `finalize-privilege-purchase-container`; falling back to `document.body` is not acceptable.
Applied to files:
webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts
📚 Learning: `privilegepurchaseacceptui` publishes payment results via `"success"` / `"error"` events; the visual...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#847
File: webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts:141-155
Timestamp: 2025-06-11T18:49:14.626Z
Learning: `PrivilegePurchaseAcceptUI` publishes payment results via `"success"` / `"error"` events; the visual feedback is rendered by the parent component (`PrivilegePurchaseFinalize.ts`), so the child intentionally does not set its own `errorMessage` / `successMessage`.
Applied to files:
webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts
📚 Learning: in licensecard component's clickunencumberitem method (webroot/src/components/licensecard/licensecar...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#873
File: webroot/src/components/LicenseCard/LicenseCard.ts:414-443
Timestamp: 2025-06-24T00:02:39.944Z
Learning: In LicenseCard component's clickUnencumberItem method (webroot/src/components/LicenseCard/LicenseCard.ts), complex event handling for checkbox interactions is intentionally designed to ensure consistent behavior across checkbox input, wrapper label, and outer selection parent elements for custom UI requirements. This complexity should be preserved rather than simplified.
Applied to files:
webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts
📚 Learning: in focus trap implementations for modals, when there's only one focusable element (like in a success...
Learnt from: jsandoval81
PR: csg-org/CompactConnect#922
File: webroot/src/components/UserAccount/UserAccount.ts:250-267
Timestamp: 2025-07-10T19:52:40.366Z
Learning: In focus trap implementations for modals, when there's only one focusable element (like in a success state), it's correct for both firstTabIndex and lastTabIndex to reference the same element. This keeps focus appropriately trapped on that single element. Optional chaining operators (?.focus()) provide adequate null safety for DOM element access in focus management code.
Applied to files:
webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts
🔇 Additional comments (8)
webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts (3)
54-55: LGTM! Properties for focus trap management.The new properties appropriately store references to DOM elements needed for focus trap functionality.
118-129: Good cleanup implementation.The method properly removes event listeners and DOM elements to prevent memory leaks.
180-197: Well-implemented focus trap logic.The method correctly handles Tab and Shift+Tab navigation, cycling focus within the payment popup when visible. This addresses the Safari keyboard navigation issue mentioned in the PR objectives.
webroot/src/components/Page/PageMainNav/PageMainNav.vue (1)
75-75: Correct implementation of dynamic class binding.The conditional class binding for iPhone Safari is properly implemented and will work with the corresponding CSS rules to address mobile Safari viewport issues.
webroot/src/components/Page/PageMainNav/PageMainNav.less (1)
58-62: Good Safari-specific fix for navigation visibility.The 8.4rem padding addresses the iPhone Safari URL bar issue effectively. This approach is consistent with your team's established pattern for handling Safari-specific layout issues.
webroot/src/components/Page/PageMainNav/PageMainNav.ts (1)
145-152: Comprehensive iPhone Safari detection.The browser detection correctly identifies iPhone Safari while excluding other iOS browsers. The implementation properly addresses the requirement to apply Safari-specific fixes only where needed.
webroot/src/components/UserAccount/UserAccount.less (2)
50-53: Effective fix for Safari modal centering.The flexbox layout with centered alignment properly addresses the email verification modal centering issue on Safari mentioned in the PR objectives.
115-126: Good responsive design for action buttons.The width adjustments (100% on mobile, auto on desktop) improve the modal's responsive behavior across different screen sizes.
rmolinares
left a comment
rmolinares
left a comment
There was a problem hiding this comment.
Nice job 👍! I did notice a few things:
- When I close the authorize.net frame using the keyboard, it goes away but I'm not able to continue tabbing on the "Payment summary" screen. I can shift+tab, but it appears that once I reach a certain point, I'm not longer able to cycle forward with just tab
- Not necessarily part of this initial scope, but when testing on Xcode simulators, I noticed the logout section is cut off a little on iPad devices. I don't know if it makes sense to add a computed prop that checks for Safari on iPad similar to the one for phone, or to just increase the
margin-bottomforul.navonPageMainNav.less.
iPad Pro (12.9 inch) 5th generation
iPad Mini 6th generation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
webroot/src/components/Page/PageHeader/PageHeader.less (1)
25-27: Align media query with pointer capability for consistency with router logicRouter now gates behavior on
(hover: hover) and (pointer: fine). To avoid divergence and accidental styling on hover-capable but coarse pointers, mirror that here.Proposed diff:
- @media @tabletWidth and (hover: hover) { + @media @tabletWidth and (hover: hover) and (pointer: fine) { background-color: transparent; }Please verify intended Safari targets support both
hoverandpointermedia features. If you need to support older Safari versions lackingpointer, consider a graceful fallback.webroot/src/components/Page/PageHeader/PageHeader.ts (1)
22-26: LGTM: Unified touch-toggle detection
isMenuTouchTogglecleanly captures phone-only or no-hover contexts and aligns with related components.To prevent logic drift across components, consider centralizing this in a small composable (e.g.,
useDeviceCapabilities.ts) that exposesisMenuTouchToggle,isTouchDevice, etc. This keeps one source of truth when future heuristics evolve (e.g., fold inany-hoverhandling).webroot/src/components/Page/PageHeader/PageHeader.vue (2)
10-21: Minor a11y: Support Space key on logo “button”For div/img with
role="button", Space should activate the control as well as Enter.Proposed diff:
<img src="@assets/logos/compact-connect-logo-white.svg" :alt="$t('common.appName')" class="logo" @click="logoClick" - @keyup.enter="logoClick" + @keyup.enter="logoClick" + @keyup.space.prevent="logoClick" role="button" tabindex="0" :aria-label="$t('common.appName')" /> ``` <!-- review_comment_end --> --- `23-33`: **A11y: Use semantic button or add aria-expanded and Space key handling** Prefer a <button> for the toggle. If keeping a div, expose state via `aria-expanded` and support Space to activate. Proposed diff: ```diff - <div + <div class="nav-toggle" :class="{ expanded: $store.state.isNavExpanded }" @click="navToggle" - @keyup.enter="navToggle" + @keyup.enter="navToggle" + @keyup.space.prevent="navToggle" role="button" tabindex="0" - :aria-label="$t('common.toggleNav')" + :aria-label="$t('common.toggleNav')" + :aria-expanded="$store.state.isNavExpanded.toString()" > ``` <!-- review_comment_end --> </blockquote></details> <details> <summary>webroot/src/components/Page/PageContainer/PageContainer.ts (1)</summary><blockquote> `43-47`: **LGTM: Expanded device heuristic** Switching from `isPhone` to `isMenuTouchToggle` broadens inclusion to non-hover devices (e.g., mobile Safari, touch tablets), matching the PR goals. - Consider centralizing this logic in a composable to keep it consistent with `PageHeader`/`PageMainNav`. - Verify header visibility on iPad with/without trackpad; `hover` can change dynamically with pointer devices. If needed, you can listen for `change` on the MediaQueryList to react to accessory attachment. <!-- review_comment_end --> </blockquote></details> <details> <summary>webroot/src/components/Page/PageMainNav/PageMainNav.ts (1)</summary><blockquote> `158-160`: **Verify broadened toggle condition on hybrid devices** `isMenuTouchToggle` now triggers for any touch device (not just phone breakpoint). This likely improves tablet behavior (goal), but please verify on hybrid laptops/convertibles (e.g., Surface, Pixelbook) to ensure menu behavior isn’t unintentionally “touch-optimized” when a hover-capable pointer is primary. Suggested manual checks: - Windows Surface (tablet + keyboard/mouse): hover/expand/collapse behavior should remain desktop-like. - iPad (no external pointing device): should use touch toggle. - iPad + trackpad: confirm it behaves desktop-like (hover-capable => not touch toggle). If issues arise, consider narrowing to “coarse pointer” in addition to `hover: none` if supported by your `$matches` plugin (e.g., `$matches.pointer === 'coarse'`). </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f98dded172faec2319b607895d8021bb8c08d8fc and 2dd0eaf8865fedcd80d44cd36e1a0c8def3e428a. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `webroot/yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (12)</summary> * `webroot/src/components/Page/PageContainer/PageContainer.less` (3 hunks) * `webroot/src/components/Page/PageContainer/PageContainer.ts` (2 hunks) * `webroot/src/components/Page/PageHeader/PageHeader.less` (1 hunks) * `webroot/src/components/Page/PageHeader/PageHeader.ts` (1 hunks) * `webroot/src/components/Page/PageHeader/PageHeader.vue` (2 hunks) * `webroot/src/components/Page/PageMainNav/PageMainNav.less` (3 hunks) * `webroot/src/components/Page/PageMainNav/PageMainNav.ts` (1 hunks) * `webroot/src/components/Page/PageMainNav/PageMainNav.vue` (2 hunks) * `webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts` (3 hunks) * `webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.vue` (1 hunks) * `webroot/src/router/index.ts` (1 hunks) * `webroot/src/shims-vue-responsiveness.d.ts` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.vue * webroot/src/components/Page/PageContainer/PageContainer.less * webroot/src/shims-vue-responsiveness.d.ts </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * webroot/src/components/Page/PageMainNav/PageMainNav.vue * webroot/src/components/PrivilegePurchaseAcceptUI/PrivilegePurchaseAcceptUI.ts * webroot/src/components/Page/PageMainNav/PageMainNav.less </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (3)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: jsandoval81
PR: #873
File: webroot/src/styles.common/_inputs.less:115-118
Timestamp: 2025-06-24T00:17:31.188Z
Learning: The team intentionally uses broad CSS selectors like.dp__input_wrap div { position: static; }to fix styling issues with the Vue Date-Picker library. They have experience with these styles working well, keep the library version pinned in yarn.lock, and have established processes to re-test everything when updating the library version. This approach is acceptable given their testing discipline and version control.Learnt from: jsandoval81
PR: #922
File: webroot/src/components/UserAccount/UserAccount.ts:0-0
Timestamp: 2025-07-10T19:50:56.745Z
Learning: In the UserAccount component (webroot/src/components/UserAccount/UserAccount.ts), the email field should be disabled for staff users (isDisabled: this.isStaff) to maintain existing restrictions, while licensees should be able to change their email address through the new verification process. This is the intended behavior per PR #922 requirements.Learnt from: jsandoval81
PR: #799
File: webroot/src/pages/LicenseeProof/LicenseeProof.less:144-168
Timestamp: 2025-05-07T18:15:17.462Z
Learning: In the CompactConnect project, print layouts for footers are designed to use fixed positioning (position: fixed) in Chrome and Firefox while using a fallback to relative positioning for Safari (detected via supports (font: -apple-system-body)). This approach is intentional and aligns with the design requirements, as Safari doesn't handle fixed elements correctly in print layouts.Learnt from: jsandoval81
PR: #922
File: webroot/src/components/UserAccount/UserAccount.ts:250-267
Timestamp: 2025-07-10T19:52:40.366Z
Learning: In focus trap implementations for modals, when there's only one focusable element (like in a success state), it's correct for both firstTabIndex and lastTabIndex to reference the same element. This keeps focus appropriately trapped on that single element. Optional chaining operators (?.focus()) provide adequate null safety for DOM element access in focus management code.</details> <details> <summary>📚 Learning: 2025-05-28T16:12:31.310Z</summary>Learnt from: jsandoval81
PR: #822
File: webroot/src/router/routes.ts:96-99
Timestamp: 2025-05-28T16:12:31.310Z
Learning: In webroot/src/router/routes.ts, the team prefers to keep router logic simple and handle route parameter validation (like state abbreviations) in the page components themselves based on user permissions, rather than adding validation at the router level.**Applied to files:** - `webroot/src/router/index.ts` </details> <details> <summary>📚 Learning: 2025-07-03T15:35:57.893Z</summary>Learnt from: rmolinares
PR: #905
File: webroot/src/components/UpdateHomeJurisdiction/UpdateHomeJurisdiction.vue:32-41
Timestamp: 2025-07-03T15:35:57.893Z
Learning: In the CompactConnect frontend codebase, the team prefers to keep non-dynamic text directly in Vue templates rather than moving it to computed properties in TypeScript modules, as this approach prevents cluttering the TS files with template labels.**Applied to files:** - `webroot/src/components/Page/PageHeader/PageHeader.vue` </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: CheckWebroot </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>webroot/src/components/Page/PageContainer/PageContainer.ts (1)</summary> `55-56`: **Behavior check: Header inclusion now depends on no-hover OR phone** Confirm that this intendedly shows the header on larger touch-only devices (tablets) and hides it on hover-capable tablets with keyboards/trackpads. If that’s desired, great. If not, consider incorporating `(pointer: coarse)` alongside `hover === 'none'` to target finger-first tablets explicitly. <!-- review_comment_end --> </details> <details> <summary>webroot/src/components/Page/PageMainNav/PageMainNav.ts (1)</summary> `145-147`: **LGTM: media-query–based touch heuristic is appropriate** Using `$matches.hover === 'none'` is a solid, reactive signal for touch-first contexts via the media query. No changes needed. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
rmolinares
left a comment
rmolinares
left a comment
There was a problem hiding this comment.
Nice work on the iPad navigation. Looks much better / as expected in every simulator device I've tested.
Regarding the tabbing after the payment modal has been closed, I am now able to tab a few times (cycling through the form buttons) but it the focus eventually gets lost again and am unable to cycle through in a loop fashion the way I am with shift+tab. With shift+tab I can infinitely cycle through, but not with just tab only. Your fix may be suffice for closing this ticket, but figured I'd bring it up in case we wanted to get that to function as normal. Thoughts on this?
|
@jlkravitz This is ready for your review. |
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
Couple small items but generally looks great. Thanks for taking the time to dive into the nitty-gritty of Safari / mobile!
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen good to merge!
4 participants
Requirements List
yarn install --ignore-enginesDescription List
vue-responsivenessdependencyTesting List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsCloses #898
Close #967
Summary by CodeRabbit
Accessibility Improvements
Bug Fixes
Style
New Features / Enhancements