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.

Sign up for GitHub

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

Jump to bottom

Fix issue whereby tooltips loaded dynamically while moving the map were never shown. #8672

Merged
merged 19 commits into from
May 18, 2023
Merged

Fix issue whereby tooltips loaded dynamically while moving the map were never shown. #8672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7da8b9e
Fix issue whereby tooltips loaded dynamically while moving/dragging w…
theGOTOguy Dec 22, 2022
3478ff1
Use chaining to simplify check whether map is dragging and moving.
theGOTOguy Nov 20, 2022
5489fd9
Use more concise function in tests.
theGOTOguy Nov 20, 2022
40034ad
Fix issue whereby tooltips loaded dynamically while moving/dragging w…
theGOTOguy Nov 19, 2022
b0ba81a
Use more concise function in tests.
theGOTOguy Nov 20, 2022
4eb38ef
Fix linter error.
theGOTOguy Dec 7, 2022
ee8a033
Fix issue whereby tooltips loaded dynamically while moving/dragging w…
theGOTOguy Nov 19, 2022
33f2e91
Fix issue whereby tooltips loaded dynamically while moving/dragging w…
theGOTOguy Nov 19, 2022
e996439
Fix issue whereby tooltips loaded dynamically while moving/dragging w…
theGOTOguy Nov 19, 2022
f325e02
Fix issue whereby tooltips loaded dynamically while moving/dragging w…
theGOTOguy Dec 22, 2022
0c834bd
Use more concise function in tests.
theGOTOguy Nov 20, 2022
18140b7
Fix issue whereby test was added multiple times during merges.
theGOTOguy Dec 22, 2022
fa40d46
Remove test "don't opens the tooltip marker moseover while dragging m…
theGOTOguy Dec 22, 2022
47f4f3d
Fix attempt to open Tooltip after dragging
Falke-Design Dec 25, 2022
5c1b7da
Merge branch 'main' into tooltip-loaded-while-dragging
theGOTOguy Jan 17, 2023
0da6694
Removes some additional nesting.
theGOTOguy Jan 17, 2023
7af5b1f
Linter fixes.
theGOTOguy Jan 17, 2023
2f22358
Merge branch 'main' into tooltip-loaded-while-dragging
Falke-Design May 16, 2023
d907ba4
Update test
Falke-Design May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 24 additions & 6 deletions spec/suites/layer/TooltipSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,9 +447,8 @@ describe('Tooltip', () => {
const tooltip = layer.getTooltip();

// simulate map dragging
map.dragging.moving = function () {
return true;
};
map.dragging.moving = () => true;

UIEventSimulator.fireAt('mouseover', 210, 195);
expect(tooltip.isOpen()).to.be.false;

Expand All @@ -472,9 +471,7 @@ describe('Tooltip', () => {
expect(tooltip.isOpen()).to.be.true;

// simulate map dragging
map.dragging.moving = function () {
return true;
};
map.dragging.moving = () => true;
UIEventSimulator.fire('mouseout', layer._icon, {relatedTarget: map._container});
expect(tooltip.isOpen()).to.be.false;

Expand All @@ -483,6 +480,27 @@ describe('Tooltip', () => {
expect(tooltip.isOpen()).to.be.false;
});

it('opens the tooltip if the tooltip is loaded while the map is dragging.', () => {
// simulate map dragging
map.dragging.moving = () => true;

// If tooltips are dynamically loaded while the map is dragging, they need
// to be loaded when the dragging stops.
const layer = L.marker(center).bindTooltip('Tooltip', {permanent: true});
map.addLayer(layer);

// simulate map not dragging anymore
map.dragging.moving = () => false;

// Actually triggers both movestart and moveend.
map.setView([51.505, -0.09], 13);

// The tooltip is loaded now!
expect(map.hasLayer(layer._tooltip)).to.be.true;
const tooltip = layer.getTooltip();
expect(tooltip.isOpen()).to.be.true;
});

it('opens tooltip with passed latlng position while initializing', () => {
const tooltip = new L.Tooltip(center)
.addTo(map);
Expand Down
9 changes: 5 additions & 4 deletions src/dom/Draggable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,18 +202,19 @@ export const Draggable = Evented.extend({
DomUtil.enableImageDrag();
DomUtil.enableTextSelection();

if (this._moved && this._moving) {
const fireDragend = this._moved && this._moving;

this._moving = false;
Draggable._dragging = false;

if (fireDragend) {
Comment on lines +207 to +210
Copy link
Member

Choose a reason for hiding this comment

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

@mourner @jonkoops @IvanSanchez do you see any problems with setting the flags to false before fireing the event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the only way this would matter would be if someone was looking at _moving or _dragging during the handling of a dragend event. We're not doing it internally to Leaflet, but it's possible that someone who depends on us might. To do so would be inadvisable, because there would be a race between the event firing and _moving and _dragging being updated after the event is fired. Maybe mark this as a breaking API change in case someone depends on the state of _moving or _dragging in an event triggered by dragend?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no issue with that, this is internal state so folks should not be relying on it anyways.

// @event dragend: DragEndEvent
// Fired when the drag ends.
this.fire('dragend', {
noInertia,
distance: this._newPos.distanceTo(this._startPos)
});
}

this._moving = false;
Draggable._dragging = false;
}

});
13 changes: 12 additions & 1 deletion src/layer/Tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,9 +413,20 @@ Layer.include({


_openTooltip(e) {
if (!this._tooltip || !this._map || (this._map.dragging && this._map.dragging.moving())) {
if (!this._tooltip || !this._map) {
return;
}

// If the map is moving, we will show the tooltip after it's done.
if (this._map.dragging && this._map.dragging.moving() && !this._openOnceFlag) {
this._openOnceFlag = true;
this._map.once('moveend', () => {
this._openOnceFlag = false;
this._openTooltip(e);
theGOTOguy marked this conversation as resolved.
Show resolved Hide resolved
});
return;
}

this._tooltip._source = e.layer || e.target;

this.openTooltip(this._tooltip.options.sticky ? e.latlng : undefined);
Expand Down