Bug: Multiple image select with boxZoom

[sashadev-sky]

Currently to select multiple images, you can use cmd + click. There is another feature, found in the file BoxSelectHandle.js (https://github.com/publiclab/Leaflet.DistortableImage/blob/main/src/edit/BoxSelectHandle.js)
that overrides Leaflet's default boxZoom to allow for using shift + drag to select multiple images.

However, there is a bug with it so currently the event listener for this functionality is commented out. To test this bug uncomment this line.

Leaflet.DistortableImage/src/DistortableCollection.js

Line 16 in 88fe928

// L.DomEvent.on(map, "boxzoomend", this._addSelections, this);

Actual behavior

• After click on the map to deselect, clicking an image in the same area as the previous boxSelect area reselects the images for multiple select (something that typically should only be triggered by cmd + click not regular click).

Expected behavior

• By leaving an image out of the boxSelect area, I demonstrate the expected behavior (no reselections on regular click):
  

Relevant lines of code

• the BoxSelectHandle.js file linked above overrides the default boxZoom handler
• in the Leaflet.DisortableCollection.js, the event listener runs the code in
  

  Leaflet.DistortableImage/src/DistortableCollection.js

  Lines 64 to 78 in 88fe928

  	_addSelections: function(e) {
  	var box = e.boxZoomBounds,
  	i = 0;
  	this.eachLayer(function(layer) {
  	var edit = layer.editing;
  	if (edit.toolbar) { edit._hideToolbar(); }
  	for (i = 0; i < 4; i++) {
  	if (box.contains(layer.getCorners()[i]) && edit._mode !== "lock") {
  	L.DomUtil.addClass(layer.getElement(), "selected");
  	break;
  	}
  	}
  	});
  	},

  to decide which images to select.
• have to figure out what is causing the reselection (event propagation, event execution deferral, etc.?) and stop it

[sashadev-sky]

@jywarren the buggy behavior I was referring to in #158

[shivam15]

can i try this one?

[sashadev-sky]

@shivam15 Yes please!! Let me know if you have any questions because I wrote the original code for it.. just couldn't get it to work properly!

[rexagod]

One more thing: If I delete an image, then try to drag the selection, it throws an error at every mouse drag movement (see below). Let's see if resolving this issue resolves this one too, otherwise I'll open a separate issue for this.

[sashadev-sky]

@rexagod I fixed this in my pending PR #229 ! I noticed that too

[sashadev-sky]

@rexagod also not the box select feature only works best if you drag from bottom-right towards top-left. Cannot explain exactly why this is but perhaps its been fixed when they updated the logic for flipping axis?

[rexagod]

leaflet.distortableimage.js:915 _deselectOthers
leaflet.distortableimage.js:904 _toggleMultiSelect
leaflet.distortableimage.js:929 _addSelections

For some reason, _addSelections is getting unexpectedly triggered after some particular functions, even after I suppress the event propogation.

[rexagod]

Okay so the "boxzoomend" is definitely not firing when it should, either that, or something else is triggering the _addSelections method.

[rexagod]

Okay so the boxzoomend by default, in the leaflet library, is triggered on the "mouseup" event. This leads to the triggering of the _addSelections whenever a "mouseup" is detected, hence causing these "reselections".

  _onMouseUp: function (e) {
    if ((e.which !== 1) && (e.button !== 1)) { return; }

    this._finish();

    if (!this._moved) { return; }
    // Postpone to next JS tick so internal click event handling
    // still see it as "moved".
    this._clearDeferredResetState();
    this._resetStateTimeout = setTimeout(bind(this._resetState, this), 0);

    var bounds = new LatLngBounds(
            this._map.containerPointToLatLng(this._startPoint),
            this._map.containerPointToLatLng(this._point));

    this._map
      .fitBounds(bounds)
      .fire('boxzoomend', {boxZoomBounds: bounds});
  }

Now I did try to unbind "mouseup" and "boxzoomend" listeners at different places in the code, and enable them back again, but there are some corner cases the eventually always remain.

Looking into this, @sashadev-sky.

[rexagod]

To further clarify on this, notice two different subroutines that occur.

• _addSelection gets executed on every single "mouseup" event
• Reselection takes place respective to the sequence in which the images were selected (was bottom-left to top-right below), thus explaining "Cannot explain exactly why this is but perhaps its been fixed when they updated the logic for flipping axis?"

[rexagod]

We're close (no reselections bug)! Just need to implement a few final changes to make sure everything's working as expected.

[rexagod]

@sashadev-sky I'm letting go of the above implementation and in fact considering some ideas that I'd like your thoughts on.

• We need to rewire the listeners on the _addSelection functionality, to avoid the confusion b/w "boxzoomend" and "mouseup" events.
• At the moment, I'm considering extended "boxzoom" functionality wired to a more convenient key binding (something other than mouseup so that it doesn't go off unexpectedly) in order to inherit the nice little box selection property while disabling the "mouseup" listener at initialize.
• This way we can actually avoid overriding the boxzoom functionality since we very well might need it in the future, or now.

[sashadev-sky]

Woah amazing! I can't imagine ever needing "boxzoom" for its original purpose, to zoom into a specific area. Seems like a very inconvenient way to just increase zoom. So I manually fire boxzoomend from the _mouseUp method, so I guess we could realistically fire it anywhere.

I think whatever we do it should definitely select them immediately (yellow highlight) without having to click on them. Is it possible to just make _addSelection listen for boxzoomend? Then there will be no confusion between them?

[rexagod]

It is possible to make _addSelection listen forboxzoomend and pardon me, but isn't that what we are doing in the lib right now? The catch here however, as I stated above, is the fact that boxzoomend by default, in the leaflet library, has been wired to the mouseup event, overriding which, is causing compatibility problem as discussed above. Therefore, I suggested some workarounds above that should make this work, while alternatively we can open up on issue regarding the same in Leaflet that could potentially save us some efforts. What do you think?

[sashadev-sky]

@rexagod can you move these changes into a new feature BR and upload a PR so I can look into what we have now?

[sashadev-sky]

so excited to get this one fixed!!!

[sashadev-sky]

closing via #314