-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Unbind tooltip remove focus listeners #9232
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Falke-Design it is really strange. I get the same tests result on my branch and on main. So I've run Then I wanted to see if I get any errors on the main branch. And I get this: Would there be a reason this happens? |
Did you do a re-build? |
I've done |
no, you did it correct. First build and then test. I will check it the next days by myself. In the mean time you can merge the latest commit from |
@Falke-Design so I've realized that I forgot to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better now 😄
Can you please try to add a test which covers your changes
src/layer/Tooltip.js
Outdated
DomEvent.on(el, 'focus', function () { | ||
this._tooltip._source = layer; | ||
this.openTooltip(); | ||
}, this); | ||
DomEvent.on(el, 'blur', this.closeTooltip, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the events here too
src/layer/Tooltip.js
Outdated
_handleFocus() { | ||
this._tooltip._source = this; | ||
this.openTooltip(); | ||
}, | ||
|
||
_handleBlur() { | ||
this.closeTooltip(); | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code exists twice: here and in _addFocusListenersOnLayer
Hi @Falke-Design, could I get an update? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for letting you wait so long.
I rewrote the functions, what do you think about it?
_addFocusListeners(remove) { | ||
if (this.getElement) { | ||
this._addFocusListenersOnLayer(this); | ||
} else if (this.eachLayer) { | ||
this.eachLayer(this._addFocusListenersOnLayer, this); | ||
const el = this.getElement(); | ||
if (el) { | ||
const onOff = remove ? 'off' : 'on'; | ||
DomEvent[onOff](el, 'focus', () => this._handleFocus(this), this); | ||
DomEvent[onOff](el, 'blur', this._handleBlur, this); | ||
} | ||
} else if (this.eachLayer && this._map.getRenderer(this)) { | ||
this.eachLayer(layer => this._addFocusListenersOnLayer(layer), this); | ||
} | ||
}, | ||
|
||
_addFocusListenersOnLayer(layer) { | ||
const el = typeof layer.getElement === 'function' && layer.getElement(); | ||
if (el) { | ||
DomEvent.on(el, 'focus', function () { | ||
this._tooltip._source = layer; | ||
this.openTooltip(); | ||
}, this); | ||
DomEvent.on(el, 'blur', this.closeTooltip, this); | ||
_handleFocus(source) { | ||
if (this._tooltip) { | ||
this._tooltip._source = source; | ||
this.openTooltip(); | ||
} | ||
}, | ||
|
||
_handleBlur() { | ||
this.closeTooltip(); | ||
}, | ||
|
||
_addFocusListenersOnLayer(layer, remove) { | ||
if (layer.getElement) { | ||
const el = layer.getElement(); | ||
if (el) { | ||
const onOff = remove ? 'off' : 'on'; | ||
DomEvent[onOff](el, 'focus', () => this._handleFocus(layer), this); | ||
DomEvent[onOff](el, 'blur', this._handleBlur, this); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do the following (or something similar if there is a better way):
_addFocusListeners
does the same for a element as_addFocusListenersOnLayer
, so we don't duplicate the code.- Then the
_handleBlur
don't need to be a function anymore, because we can callcloseTooltip
directly - We store the
named
function / listener (el._leaflet_focus_handler
) for the focus on the element, because an anonymous function / listener can't be removed withoff
_addFocusListeners(remove) {
if (this.getElement) {
this._addFocusListenersOnLayer(this, remove);
} else if (this.eachLayer && this._map.getRenderer(this)) {
this.eachLayer(layer => this._addFocusListenersOnLayer(layer, remove), this);
}
},
_addFocusListenersOnLayer(layer, remove) {
const el = typeof layer.getElement === 'function' && layer.getElement();
if (el) {
const onOff = remove ? 'off' : 'on';
if (!remove) {
// Remove focus listener, if already existing
el._leaflet_focus_handler && DomEvent.off(el, 'focus', el._leaflet_focus_handler, this);
el._leaflet_focus_handler = () => {
if (this._tooltip) {
this._tooltip._source = layer;
this.openTooltip();
}
};
}
el._leaflet_focus_handler && DomEvent[onOff](el, 'focus', el._leaflet_focus_handler, this);
DomEvent[onOff](el, 'blur', this.closeTooltip, this);
if (remove) {
delete el._leaflet_focus_handler;
}
}
},
setTimeout(() => { | ||
expect(() => UIEventSimulator.fire('focus', marker.getElement())).to.not.throw(); | ||
}, 2000); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for a LayerGroup too:
const marker = new Marker(latlng);
var lg = new LayerGroup([marker]).addTo(map);
lg
.bindTooltip('Tooltip that will be unbinded in two seconds')
.openTooltip()
.unbindTooltip();
|
||
setTimeout(() => { | ||
expect(() => UIEventSimulator.fire('focus', marker.getElement())).to.not.throw(); | ||
}, 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not wait so long. Maximum of 500ms is allowed, better is earlier
My solution for #9071