Allow edits to popup container class while the popup is not on the map

[SnailBones]

Currently, addClassName, removeClassName and toggleClassName all have no effect when called before the popup is added to the map, and all have an incorrect example.

In this P.R. I add a Popup._classList Set that keeps track of the added classes, pushing them to the DOM whenever the popup is added to the map.

At @arindam1993 and @rreusser's suggestion, the popup now uses a Set to keep track of its classes, rewriting the element classList on every update to the popup. This creates a single source of truth and prevents strange behavior when toggling a default class. However, for users who are bypassing the methods to directly manipulate the element classList, this is a breaking change.

Other changes:

• Returns for addClassName and removeClassName to allow for method chaining.
• Since refactoring, popups no longer throw errors for addinglremoving/toggling invalid strings. (Classes with whitespace instead add multiple classes to the DOM). Lines checking for errors in tests have been removed.
• Moved class maniuplation function from anchor.js into marker.js since it is no longer shared by popup.js.

Launch Checklist

• Allow addClassName to be called when popup is not on map.
• Add warnings for incorrect user actions (calling removeClassName and toggleClassName before popup is on map).
• Fix examples for removeClassName and toggleClassName
• Allow removeClassName and toggleClassName to be called when popup is not on map.
• Tests for addClassName.
• Tests for removeClassName and toggleClassName.
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Popup.addClassName, removeClassName and toggleClassName work while the popup is not on the map, and no longer throw DOM errors for invalid class names. Directly modifying the popup container CSS class no longer works, all edits should use these functions. </changelog>
• Implement Set with one-way update.

[arindam1993]

I'd be a fan of moving the classList out from the container and onto an array(or Set maybe? 🤔 ) on the Popup instance itself.
That way we could lift the restriction on remove and toggle as well, and implement a one-way update form this.classList -> this._container.classList whenever changes are triggered.

[karimnaaji]

@SnailBones I believe the failed CI tests are unrelated to your PR. I would pull in @arindam1993 's change from yesterday (780a0bb) either rebasing or merging from latest main, as you prefer.

[SnailBones]

I'd be a fan of moving the classList out from the container and onto an array(or Set maybe? 🤔 ) on the Popup instance itself.
That way we could lift the restriction on remove and toggle as well, and implement a one-way update form this.classList -> this._container.classList whenever changes are triggered.

@arindam1993 What do you think of the implementation with another DOM element here?

[rreusser]

Leaving some super nitpicky details as a review, but I rather like @arindam1993's suggestion of using a Set here. It's not without precedence in GL JS, though I'm not 100.0% sure this usage was definitely intentional and didn't just happen to sneak through:

mapbox-gl-js/src/util/mapbox.js

Line 573 in 780a0bb

const authenticatedMaps = new Set();

I guess spread with ... works as well for concatenating…

classList = new Set()
// Set(0) {}
classList.add('foo')
// Set(1) {"foo"}
classList.add('bar')
// Set(2) {"foo", "bar"}
[...classList].join(' ')
// "foo bar"

[SnailBones]

classList = new Set()
// Set(0) {}
classList.add('foo')
// Set(1) {"foo"}
classList.add('bar')
// Set(2) {"foo", "bar"}
[...classList].join(' ')
// "foo bar"

@rreusser That's a clever implementation! I'm still struggling to understand what the use case is though. Do you have an example of when it might be necessary to manipulate classes in a popup before it has been added to the map?

[rreusser]

Ah, I wasn't 100% certain whether we wanted to preemptively create all of the DOM elements (I imagine they're relatively lightweight, but how many would people create?), so I thought maybe instead of lifting the DOM element's classList onto an instance property (link below, from the PR), we could maybe just track the classList as a set, then concatenate and set it on an inserted DOM element directly whenever it changes.

https://github.com/mapbox/mapbox-gl-js/pull/10143/files#diff-de43205af4d4066c2e22519c9f5fa5e52ff160ed43091938190addbe4b46f4a0R117

[ryanhamley]

I can imagine a situation in which popups are set up with a default theme and users can change that theme before they're added to the map, which would make remove and toggle useful. I agree with the idea of maintaining a class list per popup instance that can be updated without a DOM element and then the DOM element can be updated as is if it exists. IMO, we should avoid creating DOM elements unless absolutely necessary since DOM creation can be time- and memory-consuming.

[rreusser]

Looks good! I'm somewhere between "request changes" and "approve" since the PR as-is looks good to me, but if it's reasonable to remove a source of truth and simply overwrite the container classes whenever they change, that would be my top preference, if only to avoid a potential corner case in which they get out of sync.

[rreusser]

What do you think about overwriting classes with something like this._applyClassList() which would look like:

_applyClassList () {
  if (!this._container) return;
  this._container.className = [...this._classList.values()].join(' ');
}

My main concern is that we have two independent sources of truth which we hope remain synchronized. But since we don't allow a user-supplied container anyway, so I don't think there should be a case where this would prevent you from accomplishing some behavior you couldn't simply achieve by using the supplied API.

However, this could be a breaking change if someone was previously manipulating the container class directly.

[SnailBones]

I don't think there's a valid reason for anyone to manipulate the container class directly right now. (Still doesn't mean that no one's doing it.)

It wouldn't be quite as simple as overwriting: the container element has additional default classes. So this:

 const popup = new Popup({className: 'some classes'});

creates this element:

 <div class="mapboxgl-popup mapboxgl-popup-anchor-bottom yellow"></div>

[rreusser]

Good point! Just wanted to be sure.

[SnailBones]

I did find one weird edge case: if the user toggles one of the default classes:

popup.toggleClassName('mapboxgl-popup');

This removes the item from the container while adding it to the list.

I'm sure we could fix this, but I'm not sure what the correct behavior would be or if it's worth the lift and added complexity just to solve a very specific edge case.

[rreusser]

If we overwrote classes, then I think this._container.className = [...this._classList.values()].join(' ') would be the simplest way to accomplish this.

[rreusser]

Nice work on this, @SnailBones! Switching my review to approve. 👍 👏

[SnailBones]

Since DOM is no longer directly updated, these functions no longer throw an error.

[SnailBones]

Seemed like a strange case for recursion here

[rreusser]

I believe I've seen this style, especially in the symbol code, where it runs modified values through a different branch of the same function, but TBH I find it slightly hard to follow, so that if otherwise equivalent, I prefer your change 👍

[karimnaaji]

@SnailBones You added a 'breaking change' label to this PR, is this breaking only in the way that popups were throwing errors on adding/removing/toggling and not anymore or is it something else?

If it's breaking and requires user code changes, is there any transition that our users should follow once we release concerning this behavior? I'm interested to know the extent of this breaking change and whether it warrants an extra note on the CHANGELOG.

[SnailBones]

Two breaking changes.

First as you mention, the lack of throwing errors on invalid popup class names.

Also, if a user has directly modified classList/className instead of using the popup functions, this change means that updates will reset classes added or removed in this way. The fix here would be ensuring that all modifications to the CSS class are performed with the three functions.

I think that both of these breaking changes should only affect very rare edge cases. The former could be an issue if an app allowed for custom popup CSS classes (I can't think of a good reason for that, but maybe there's a reason it was in the unit tests.) The latter is something that users shouldn't be doing, but I don't think we can say with certainty that they're not.

[SnailBones]

Just updated the changelog to:
Popup.addClassName, removeClassName and toggleClassName work while the popup is not on the map, and no longer throw DOM errors for invalid class names. Directly modifying the popup container CSS class no longer works, all edits should use these functions.

[arindam1993]

Nice cleanup @SnailBones.
Just one more suggestion, otherwise its a 🟢 from me!
I don't think this counts as a breaking change since it would be a bit weird if someone was actually relying on classnames getting lost.

[rreusser]

Looks great! Despite having pointed out a couple nits, this looks great, and I'd opt to merge as-is.

I think @arindam1993 is probably right that we're fine not considering this a breaking change. In some sense, literally any fix is breaking if people relied upon broken behavior, so it's probably safe to explain it clearly in the changelog but consider it really just a fix for something people may have been doing which would have had undefined results.

[rreusser]

I think this could be rearranged to reduce a set allocation (spread to arrray, then .push optional classes, then join), but it's so far down in the list of memory usage, that I fully support the above 👍

[SnailBones]

Good idea!

[rreusser]

Nit: Is there a reason for inverting the conditional? Early return can be nice to de-nest code when otherwise equivalent. Nothing blocking here for me though.

[SnailBones]

My thinking was to remove the early return allowing this. _updateClassList() to be called in any condition. I realize there's another call inside here, I'll double-check and see if I can remove that.

[SnailBones]

Yup it was redundant! (Oops) Fixed in latest commit.

[rreusser]

I believe I've seen this style, especially in the symbol code, where it runs modified values through a different branch of the same function, but TBH I find it slightly hard to follow, so that if otherwise equivalent, I prefer your change 👍