Correct what the activeElement branches actually cover - #609
Conversation
The comment said a map click leaves activeElement at <body>. It does not on the default OSM provider: Leaflet's keyboard handler sets tabIndex on the map container and focuses it on mousedown, and OpenStreetMapProvider.initMap does not pass keyboard: false, so the click lands focus on the container. The behaviour is right either way, focus follows the thing the user clicked. Only the stated reason was wrong, so the <body> branch is described as what it is: non-focusable chrome, not the map.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change rewrites comments in ChangesFocus restoration
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This update clarifies focus-restoration documentation without changing application behavior, so it presents no current merge-readiness risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Coverage is 93.459% — omlahore:fix/favorites-focus-comment into OneBusAway:develop. No base build found for |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Comment-only, and correct. I checked the claim rather than taking it on faith: OpenStreetMapProvider.svelte.js:98 builds the map as this.L.map(element, { zoomControl: false }) with no keyboard: false, so Leaflet's default keyboard handler does set tabIndex on the container and take focus on mousedown. The old comment's claim that a map click leaves activeElement at <body> was wrong on the default provider.
Splitting the <body> and rootEl branches apart also makes the actual reasoning legible — they cover genuinely different cases and the old single sentence blurred them together.
Thanks for folding the correction back in as its own change rather than leaving it buried in the #605 thread.
3 participants
Folding in the correction from the review on #605, comment only, no behaviour change.
The claim that a map click leaves
activeElementat<body>is wrong on the default provider. I checked each step rather than taking it on faith:.env.example:31setsPUBLIC_OBA_MAP_PROVIDER="osm"OpenStreetMapProvider.svelte.js:98builds the map asthis.L.map(element, { zoomControl: false }), sokeyboardstays at Leaflet's default oftrueleaflet/src/map/handler/Map.Keyboard.js:42-46doesif (container.tabIndex <= 0) { container.tabIndex = '0'; }and bindsmousedownto_onMouseDown, which focuses the containerMapView.svelte:406is<div id="map" bind:this={mapElement}></div>, no tabindex of its ownSo an OSM map click puts focus on the map container, a focusable element outside
rootEl, andrestoreFocusisfalsethere. That is the behaviour you want, focus follows what the user clicked, and the container is arrow-key pannable so nothing is stranded. Only the reason given was wrong.The comment now describes the
<body>branch as what it actually covers, non-focusable chrome such as the mobile in-flow area, and keeps therootElbranch separate since that one is about focus stranded inside the closing panel.FavoritesFloatingControl.test.jsstill passes, 9 of 9, and prettier is clean.Summary by CodeRabbit