Skip to content
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

Debounce ResizeObserver's call to map.resize() #2973

Closed
wants to merge 4 commits into from

Conversation

neodescis
Copy link
Collaborator

@neodescis neodescis commented Aug 12, 2023

  • Closes White flickering on resize in version 3.x.x #2971
  • Debounces the map's ResizeObserver's call to map.resize(), to 30 milliseconds
  • Leaves ResizeObserver's leading edge calls (i.e. initial calls before debouncing) to map.resize() in place, meaning that the first resizes before debouncing are immediate
  • Resizing in Chrome and Firefox behaves considerably better, with much less flicker

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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2023

Codecov Report

Patch coverage: 93.75% and project coverage change: +0.01% 🎉

Comparison is base (4619234) 75.01% compared to head (5caea43) 75.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2973      +/-   ##
==========================================
+ Coverage   75.01%   75.03%   +0.01%     
==========================================
  Files         240      240              
  Lines       19110    19124      +14     
  Branches     4311     4315       +4     
==========================================
+ Hits        14336    14350      +14     
  Misses       4774     4774              
Files Changed Coverage Δ
src/ui/map.ts 83.09% <93.75%> (+0.16%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

this._resizeObserver = new ResizeObserver((entries) => {
let resizeDebounceTimerId: ReturnType<typeof setTimeout> | null = null;
const resizeDebounceTime = 30;
const shouldResize = () => this._trackResize && !this._removed;
Copy link
Collaborator Author

@neodescis neodescis Aug 12, 2023

Choose a reason for hiding this comment

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

Calling resize() is no longer necessarily synchronous, so need to check if the map has been removed each time

@@ -638,14 +638,33 @@ export class Map extends Camera {
if (typeof window !== 'undefined') {
addEventListener('online', this._onWindowOnline, false);
let initialResizeEventCaptured = false;
this._resizeObserver = new ResizeObserver((entries) => {
let resizeDebounceTimerId: ReturnType<typeof setTimeout> | null = null;
const resizeDebounceTime = 30;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hard-coded value, at least for now

if (resizeDebounceTimerId) {
clearTimeout(resizeDebounceTimerId);
} else if (shouldResize()) {
resizedImmediately = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If nothing was queued, then resize immediately and force the next resize to wait

}, resizeDebounceTime);
}
if (!resizedImmediately && shouldResize()) {
resizeDebounceTimerId = setTimeout(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If an immediate resize didn't happen, then queue one

@HarelM
Copy link
Collaborator

HarelM commented Aug 13, 2023

I think there's a throttle utility somewhere in the code.

@neodescis
Copy link
Collaborator Author

neodescis commented Aug 13, 2023

There is, but throttle and debounce are not the same, and debounce seemed more appropriate here as it will wait while the user is resizing. If you think throttle would be better though, I could switch it, but the throttle utility would need an update to handle arguments to the function.

What do you think about using lodash for these? I know lodash is large, but it is possible to pull in only what we want.

@HarelM
Copy link
Collaborator

HarelM commented Aug 13, 2023

I would like to avoid adding lodash just for this although it is super useful in general.
Throttle seems fairly reasonable to use here I believe, I don't have strong feelings either way, it might be that we need to remove all kinds of utility functions and add the relevant losadh ones, as they are probably better maintained there, IDK, I'm on the fence here...

@neodescis
Copy link
Collaborator Author

neodescis commented Aug 13, 2023

I can try using throttle instead and let you know. The time might need to be higher than for debounce too though, since it's "guaranteed" to fire that often.

Understood on lodash. The only reason I mentioned it is for the reason you stated: it's well-tested and maintained.

@neodescis
Copy link
Collaborator Author

neodescis commented Aug 14, 2023

There are a few problems with the existing throttle() implementation in the codebase, but I think they can be overcome.

  1. As stated above, there is no support for the throttled function to have arguments. This is easy to add, and I've got something working for it.
  2. The function's return type is overridden to return a timer ID from setTimeout(). This isn't a problem for throttling ResizeObserver, but could be a problem in the future.
  3. It does not support leading-edge (immediate) execution. Even the first call is made to wait. I do think we would want that to happen here, so that a programmatic resize of the container would resize the map immediately.

The only other usage of throttle() so far is in the Hash class. I think that might benefit from a leading-edge execution as well, so I can go ahead and change throttle() to invoke immediately the first time, unless you think that's a problem.

@HarelM
Copy link
Collaborator

HarelM commented Aug 14, 2023

As long as the code is written in the utilities file I'm fine with the solution 🙂

@neodescis
Copy link
Collaborator Author

neodescis commented Aug 14, 2023

Closing in favor of #2986

Also, never mind on #3 above... I forgot the initial execution of the resize handler was being eaten on purpose!

@neodescis neodescis closed this Aug 14, 2023
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.

White flickering on resize in version 3.x.x
3 participants