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

Tooltips throw exceptions when using vector shapes with the canvas renderer after #8247 #8442

Closed
4 tasks done
alex4401 opened this issue Sep 15, 2022 · 14 comments · Fixed by #8498
Closed
4 tasks done
Labels
blocker Critical issue or PR that must be resolved before the next release bug
Milestone

Comments

@alex4401
Copy link

alex4401 commented Sep 15, 2022

Checklist

  • I've looked at the documentation to make sure the behavior isn't documented and expected.
  • I'm sure this is an issue with Leaflet, not with my app or other dependencies (Angular, Cordova, React, etc.).
  • I've searched through the current issues to make sure this hasn't been reported yet.
  • I agree to follow the Code of Conduct that this project adheres to.

Steps to reproduce

  1. Use latest main (9d1a841 at the time of creating this issue)
  2. Set up a map with canvas renderer and a vector shape, like a rectangle.
  3. Bind a tooltip to the vector. Check JS console.
  4. Move mouse over the vector. Check JS console.
  5. Move mouse out of the vector. Tooltip does not disappear.

Expected behavior

Tooltip does not fail to initialise or when reacting to events. They do not require DOM elements to function.

Current behavior

Exceptions thrown on: tooltip initialisation, mouse events due to getElement calls introduced in #8247 671ebdf on a canvas-rendered shape (getElement is defined on the class, but has no element to return).

Minimal example reproducing the issue

https://codepen.io/alex4401/pen/XWqpKMW

Environment

  • Leaflet version: main 9d1a841
  • Browser (with version): Firefox 104.0.2, but got user reports from all other browsers
  • OS/Platform (with version): Arch Linux
@alex4401 alex4401 added bug needs triage Triage pending labels Sep 15, 2022
@alex4401 alex4401 changed the title Tooltips throw exceptions when using vector shapes with the canvas renderer Tooltips throw exceptions when using vector shapes with the canvas renderer after #8247 Sep 15, 2022
@IvanSanchez
Copy link
Member

Can confirm.

@IvanSanchez IvanSanchez added needs solution and removed needs triage Triage pending labels Sep 15, 2022
@Malvoz
Copy link
Member

Malvoz commented Sep 25, 2022

Quite an oversight, should be fixed in #8251 by utilizing Canvas Sub DOM for accessible vectors:

leaflet-canvas-sub-dom

/cc @alekzvik

@jonkoops
Copy link
Collaborator

This seems like something we might want to consider back-porting a fix for, WDYT? Also is 1.9.1 affected?

@alex4401
Copy link
Author

Also is 1.9.1 affected?

Yup: https://codepen.io/alex4401/pen/VwxrJaQ

@jonkoops jonkoops added this to the 1.9.x milestone Sep 26, 2022
@jonkoops jonkoops added the blocker Critical issue or PR that must be resolved before the next release label Sep 26, 2022
@jonkoops
Copy link
Collaborator

Thanks for checking @alex4401.

@jonkoops
Copy link
Collaborator

@Malvoz do you think we could back-port #8369 and include only the fix for this issue, without also including the new functionality?

@moosetraveller
Copy link

I came across with a similar issue by using addData when using a canvas, a separate overlay pane, and a tooltip. I assume that the issue's origin is the same source than for this issue. If my assumption is correct and if #8490 will be fixed with the bugfix for this issue, please feel free to close #8490.

@alex4401
Copy link
Author

Yeah, that's the exact same stack trace I had. Forgot to include it above.

@mourner
Copy link
Member

mourner commented Sep 30, 2022

@jonkoops should we release v1.9.2 without the fix, or wait until this gets resolved? Not sure how involved this would be.

@Falke-Design
Copy link
Member

@mourner @jonkoops I will share tomorrow morning a fix

@Falke-Design
Copy link
Member

@mourner how should I do with making a PR for 1.9.X? Creating a PR against v1 and one for main?

@mourner
Copy link
Member

mourner commented Sep 30, 2022

Just make a v1 PR, we can cherry-pick later

@Falke-Design
Copy link
Member

@mourner @jonkoops I think 1.9.2 is ready

@jonkoops
Copy link
Collaborator

jonkoops commented Oct 1, 2022

@Falke-Design Yeah, I am going to go ahead and prep the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Critical issue or PR that must be resolved before the next release bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants