Warn when custom Marker element overrides position to non-absolute#13666
Open
ITSoFRISCHial wants to merge 1 commit intomainfrom
Open
Warn when custom Marker element overrides position to non-absolute#13666ITSoFRISCHial wants to merge 1 commit intomainfrom
ITSoFRISCHial wants to merge 1 commit intomainfrom
Conversation
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.
|
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:
Please respond to any review comments on this PR. For more details, see CONTRIBUTING.md. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mapbox positions HTML markers via
transform: translate(...)on aposition: absoluteroot (provided by the built-in.mapboxgl-markerclass). When a user-supplied element overridespositionto anything that participates in normal flow — most commonlyposition: 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— inMarker#addTo, after the element is appended to the canvas container, readgetComputedStyle(this._element).position. If it isn'tabsoluteorfixed, emit a one-timewarnOncewith the offending value and remediation hint. Skipped for the default marker since we control that element.options.element— documents theposition: absoluteconstraint and recommends movingposition: relativeto a child of the marker root.test/unit/ui/marker.test.ts— adds a test that asserts (a) a custom element withposition: relativetriggers the warning, and (b) one withposition: absolutedoes 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). Removingposition: relativefrom the marker root immediately fixed it. With this PR, the same misuse logs:Launch Checklist
test/unit/ui/marker.test.ts; full marker suite (78 tests) green locally;npm run tscandnpm run lintclean.Markeroptions.element.getComputedStyleread peraddTocall, dwarfed by adjacent layout work.@mapbox/map-design-team@mapbox/static-apisif this PR includes style spec API or visual changes. N/A.@mapbox/gl-nativeif this PR includes shader changes or needs a native port. N/A.@mapbox/gl-nativeif this PR disables any test because it also needs to be disabled on their side. N/A.gl-nativeto 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.