Fallback findCanvasEventTarget to findEventTarget #22959
Conversation
src/library_html5.js
Outdated
| @@ -362,13 +362,9 @@ var LibraryHTML5 = { | |||
| return GL.offscreenCanvases[target.substr(1)] // Remove '#' prefix | |||
| // If not found, if one is querying by using DOM tag name selector 'canvas', grab the first | |||
| // OffscreenCanvas that we can find. | |||
| || (target == 'canvas' && Object.keys(GL.offscreenCanvases)[0]) | |||
| || (target == 'canvas' && Object.keys(GL.offscreenCanvases) && GL.offscreenCanvases[Object.keys(GL.offScreenCanvases)[0]]) | |||
There was a problem hiding this comment.
This line looks like it might be more complex that it needs to be.
About just target == 'canvas' && Object.values(GL.offscreenCanvases)[0]?
Can we make this a separate PR? Ideally with a test. I guess we have very little testing for this since the existing code seems simply broken?
There was a problem hiding this comment.
I agree with that change and will push it here, but I think this whole lookup thing is pretty hacked together/"best effort".
|
As for a test which shows the bug, I guess there are two bugs here---one is that there's no use of specialEventTargets, and the other is that tag lookups are broken. I'll prepare one parameterized test for findCanvasEventTarget, varying on whether it's an ID, a tag, or a special target, both before and after an offscreen canvas has been created. |
|
I've added a set of tests that fails when the old function definition is in place, and passes with the new definition. |
JoeOsborn
force-pushed
the
22942-findCanvasEventTarget
branch
from
f9c7414
to
fc261ee
Compare
…ndEventTarget, and fix default canvas lookup bug
JoeOsborn
force-pushed
the
22942-findCanvasEventTarget
branch
from
fc261ee
to
d89f1dd
Compare
| #else | ||
| || document.querySelector(target); | ||
| #endif | ||
| || (target == 'canvas' && Object.values(GL.offscreenCanvases)[0]) |
There was a problem hiding this comment.
Is this also missing the .canvas similar to in #22958
There was a problem hiding this comment.
I was worried it would, but in fact we want the whole offscreenCanvases entry because the caller might need the shared canvas pointer. All of the callers do a check for the presence of an .offscreenCanvas field.
|
I mentioned this on Discord, but we actually have a big problem here: specialHTMLTargets is likely to only have reasonable values on the main thread, since it isn't shared across workers and probably will be configured using MAIN_THREAD_EM_ASM or something (at any rate, it's possible it would be set up on one thread and used on a different thread, which has the same issue). So a findCanvasEventTarget (or indeed a findEventTarget) won't do what we want for canvases that are referred to by specialHTMLTarget names. Only ID lookups or tag lookups actually work. It seemed like it was working because !canvas was the suggested special name and the ID of the objects involved has typically been #canvas, and the offscreenCanvases array is keyed by the ID of the element involved. So stripping the first character from the ID ends up being like a pun in that case. What's the right solution? To key offscreenCanvases based on target, rather than canvas ID? To ignore specialHTMLTargets completely in findCanvasEventTarget? |
Also fix canvas tag lookup bug, which would return a string rather than an offscreenCanvases entry.
Fixes #22942 , seemingly with no other patches needed; call sites already have to handle a variety of object shapes out of this function.