Skip to content

Fix excessive attribution control rerendering #5673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 25, 2025

Conversation

viernullvier
Copy link
Contributor

@viernullvier viernullvier commented Mar 25, 2025

The lead-up

In one of my projects I noticed a weird flickering on the attribution control that seemed to be related to rapid data updates. Notably, it only occurred in Firefox and not in Chromium, so I started to investigate.

The issue

Since attribution strings can include untrusted HTML, PR #5057 introduces a bit of input sanitization to mitigate potential XSS attacks trough the attribution field. The attribution control includes a mechanism to suppress excessive re-renders, but #5057 introduced a bug: The suppression logic compares the cached sanitized result with the new unsanitized result. This will lead to excessive DOM updates if the sanitized attribution actually differs from the unsanitized one, for instance if it contains a © HTML entity that gets transformed into a © literal.

The fix

This PR reverts some of the changes in #5057: The attribution control will cache the unsanitized input again instead of the sanitized result and only call the sanitizer directly when updating the DOM. This might even bring a minor performance improvement since the sanitizer will no longer be called on every update, but only if the unsanitized input has been changed.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@viernullvier viernullvier marked this pull request as draft March 25, 2025 19:49
@viernullvier viernullvier force-pushed the attribution-control-rerender branch from e7c8788 to 73a58ff Compare March 25, 2025 20:09
@HarelM
Copy link
Collaborator

HarelM commented Mar 25, 2025

Thanks!
Ping me when this is ready for review as I don't get notified when a PR status is changed from drag to ready for review.

@viernullvier viernullvier marked this pull request as ready for review March 25, 2025 20:46
@viernullvier
Copy link
Contributor Author

@HarelM I think it's ready for review if you don't mind the lack of benchmarks - the headless benchmarks keep crashing on my machine and the in-browser ones are causing out-of-memory issues.

@HarelM
Copy link
Collaborator

HarelM commented Mar 25, 2025

Benchmark results for the attribution control is not interesting.

Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM THANKS!

@HarelM HarelM merged commit 8b5f926 into maplibre:main Mar 25, 2025
17 checks passed
@viernullvier viernullvier deleted the attribution-control-rerender branch March 25, 2025 20:57
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.

2 participants