Skip to content

Warn when custom Marker element overrides position to non-absolute#13666

Open
ITSoFRISCHial wants to merge 1 commit intomainfrom
warn-on-non-absolute-marker-position
Open

Warn when custom Marker element overrides position to non-absolute#13666
ITSoFRISCHial wants to merge 1 commit intomainfrom
warn-on-non-absolute-marker-position

Conversation

@ITSoFRISCHial
Copy link
Copy Markdown

@ITSoFRISCHial ITSoFRISCHial commented Apr 30, 2026

Summary

Mapbox positions HTML markers via transform: translate(...) on a position: absolute root (provided by the built-in .mapboxgl-marker class). When a user-supplied element overrides position to anything that participates in normal flow — most commonly position: relative, which front-end developers add reflexively to create a positioning context for absolutely-positioned descendants like ripple-ring effects — the marker root re-enters the canvas container's normal flow. The first marker lands correctly; each subsequent marker stacks below the previous one in DOM order, so pins after the first appear visually offset and drift as the camera moves (transform tracks the projection from the wrong origin).

The mechanism is subtle, the conflict is undocumented, and there's no lint or type check that catches it. This PR adds a runtime warning at the point of the misuse so developers get a one-line hint instead of debugging a phantom projection bug.

What changed

  • src/ui/marker.ts — in Marker#addTo, after the element is appended to the canvas container, read getComputedStyle(this._element).position. If it isn't absolute or fixed, emit a one-time warnOnce with the offending value and remediation hint. Skipped for the default marker since we control that element.
  • JSDoc on options.element — documents the position: absolute constraint and recommends moving position: relative to a child of the marker root.
  • test/unit/ui/marker.test.ts — adds a test that asserts (a) a custom element with position: relative triggers the warning, and (b) one with position: absolute does not.

No public API or visual behavior changes; the warning is the only externally observable effect.

Why a warning rather than a DOM-restructure fix

A DOM wrapper-div around user elements would eliminate the bug class entirely, but it'd be a breaking change for anyone using descendant selectors like .mapboxgl-marker > .my-pin. A warning is non-breaking, costs ~10 lines, and points the developer at the exact rule to move.

Repro

Originally hit while building mapbox/city-voting-tool — a globe-projection map where each submitted city drops an HTML marker. Pin 1 was correct; pin 2+ landed ~41px south of the correct latitude (one marker height per preceding marker). Removing position: relative from the marker root immediately fixed it. With this PR, the same misuse logs:

Marker element has computed CSS \position: relative`, expected `absolute`. Mapbox positions markers via `transform` on a `position: absolute` root; overriding it (often via a user style like `position: relative` on the marker root) breaks placement of additional markers. Move that rule to a child of the marker element instead.`

Launch Checklist

  • Make sure the PR title is descriptive and preferably reflects the change from the user's perspective.
  • Add additional detail and context in the PR description (with screenshots/videos if there are visual changes).
  • Manually test the debug page. Verified the new warning fires only on misconfigured custom elements; default markers and correctly-styled custom elements emit nothing.
  • Write tests for all new functionality and make sure the CI checks pass. Added unit test in test/unit/ui/marker.test.ts; full marker suite (78 tests) green locally; npm run tsc and npm run lint clean.
  • Document any changes to public APIs. Updated JSDoc on Marker options.element.
  • Post benchmark scores if the change could affect performance. N/A — one getComputedStyle read per addTo call, dwarfed by adjacent layout work.
  • Tag @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes. N/A.
  • Tag @mapbox/gl-native if this PR includes shader changes or needs a native port. N/A.
  • Tag @mapbox/gl-native if this PR disables any test because it also needs to be disabled on their side. N/A.
  • Create a ticket for gl-native to groom in the MAPSNAT JIRA queue if this PR includes shader changes or features not present in the native side or if it disables a test that's not disabled there. N/A.

Mapbox positions markers via `transform: translate(...)` on a
`position: absolute` root (provided by `.mapboxgl-marker`). When a
user-supplied element overrides `position` (e.g. `position: relative`,
which CSS authors reflexively add to create a positioning context for
absolutely-positioned descendants), the marker re-enters the canvas
container's normal flow. The first marker is correct; each subsequent
marker stacks below the previous one, so pins 2+ land at incorrect
screen positions.

Detect this in `Marker#addTo` by reading the computed `position` after
the element is attached. If it isn't `absolute` or `fixed`, emit a
`warnOnce` with the offending value and a remediation hint. Skipped for
the default marker since we control its element.

Also updates the JSDoc on `options.element` to document the constraint.
@ITSoFRISCHial ITSoFRISCHial requested a review from a team as a code owner April 30, 2026 03:50
@ITSoFRISCHial ITSoFRISCHial requested review from underoot and removed request for a team April 30, 2026 03:50
@github-actions
Copy link
Copy Markdown

Hey, @ITSoFRISCHial 👋 Thanks for your contribution to Mapbox GL JS!

Important: This repository does not accept direct merges. All changes go through our internal review process.

What happens next:

  1. A team member will review your PR here first
  2. If it looks good, they will import it to our internal repository for further review
  3. If approved, changes will be synced back here via our release process

Please respond to any review comments on this PR. For more details, see CONTRIBUTING.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant