fix: Prevent toolbar from overlapping the right-click context menu#11881
fix: Prevent toolbar from overlapping the right-click context menu#11881mykh-hailo wants to merge 3 commits intoopenstreetmap:developfrom
Conversation
mykh-hailo
changed the title
tyrasd
left a comment
tyrasd
left a comment
There was a problem hiding this comment.
This looks good to me, thanks!
Just a few code linting remarks (see below) that should be easy to address.
PS: I was wondering why the top/bottom branch can get away with using the slightly simpler implementation using offsetWidth / w properties of the popover/scroll nodes, while for left/right we have to use getBoundingClientRects to calculate the clamped positions. I'd assume the getBoundingClientRect approach would also work for the top/bottom case, and it should be more robust in case we want to use it in the future for a situation where the simple calculation might not work. Also, maybe the code could be simplified slightly when the two branches use the same calculation (only x/y need to be flipped for each)… What do you think?
modules/ui/popover.js
Outdated
| if (scrollNode) { | ||
|
|
||
| var initialPosX = position.x; | ||
| if ((placement === 'top' || placement === 'bottom')) { |
There was a problem hiding this comment.
double parentheses are not necessary here:
| if ((placement === 'top' || placement === 'bottom')) { | |
| if (placement === 'top' || placement === 'bottom') { |
modules/ui/popover.js
Outdated
| position.y += (scrollNodeRect.top + 10) - popoverRect.top; | ||
| } | ||
|
|
||
| var arrow = anchor.selectAll('.popover-' + _id + ' > .popover-arrow'); |
There was a problem hiding this comment.
this var arrow is already defined above in the if (placement === 'top' || placement === 'bottom') branch. Maybe change both to const.
const arrow = anchor.selectAll('.popover-' + _id + ' > .popover-arrow');(And since we're touching this part of the code anyway: can you also upgrade the other remaining variable definitions in the two if branches here to consts?)
modules/ui/popover.js
Outdated
| // keep the arrow centered on the button, or as close as possible | ||
| var arrowPosX = Math.min(Math.max(popoverFrame.w / 2 - (position.x - initialPosX), 10), popoverFrame.w - 10); | ||
| arrow.style('left', ~~arrowPosX + 'px'); | ||
| } else if (placement === "left" || placement === "right") { |
There was a problem hiding this comment.
for strings, please use single brackets '.
| } else if (placement === "left" || placement === "right") { | |
| } else if (placement === 'left' || placement === 'right') { |
modules/ui/popover.js
Outdated
| var arrowPosX = Math.min(Math.max(popoverFrame.w / 2 - (position.x - initialPosX), 10), popoverFrame.w - 10); | ||
| arrow.style('left', ~~arrowPosX + 'px'); | ||
| var popoverRect = popoverSelection.node().getBoundingClientRect(); | ||
| var scrollNodeRect = scrollNode.getBoundingClientRect() |
There was a problem hiding this comment.
missing ; 😉
| var scrollNodeRect = scrollNode.getBoundingClientRect() | |
| var scrollNodeRect = scrollNode.getBoundingClientRect(); |
|
@tyrasd I made some updates on your comments on my PR. |
2 participants
What is this PR
This branch fixes popover placement when using left/right placements inside scrollable containers. The update adjusts the popover's vertical position to prevent overflow beyond the scroll container, clamps the arrow position so it remains visually aligned and within the popover bounds, and includes a short-lived debug log used during verification
Closes: #11017
Root Cause
over-map) and the arrow could be mis-positioned or clipped, especially when the anchor or container was scrolled or the popover size changed dynamically.What changed