[BUG] pys-onClick attribute does not work, when element has no id #359

yannickfunk · 2022-05-13T14:24:21Z

Describe the bug
When you whant to use the pys-onClick attribute to define a python function as the click listener, and the html element has no id, it does not work.

Error:

pyodide.asm.js:14 Uncaught (in promise) PythonError: Traceback (most recent call last):
  File "/lib/python3.10/asyncio/futures.py", line 201, in result
    raise self._exception
  File "/lib/python3.10/asyncio/tasks.py", line 232, in __step
    result = coro.send(None)
  File "/lib/python3.10/site-packages/_pyodide/_base.py", line 500, in eval_code_async
    await CodeRunner(
  File "/lib/python3.10/site-packages/_pyodide/_base.py", line 351, in run_async
    coroutine = eval(self.code, globals, locals)
  File "<exec>", line 1, in <module>
  File "<exec>", line 141, in element
JsException: SyntaxError: Failed to execute 'querySelector' on 'Document': '#' is not a valid selector.

To Reproduce
Using:

Chrome Version 101.0.4951.67
Firefox Version 100.0 (64-bit)

Use this html and click on the button:

<!DOCTYPE html>
<html lang="en">
<head>
    <script defer src="https://pyscript.net/alpha/pyscript.js"></script>
</head>

<body>
	<py-script>
from js import console
def clicked(*args, **kwargs):
	console.log("clicked")
	</py-script>
	
	<button pys-onClick="clicked">Click me!</button>
	
</body>
</html>

Expected behavior
The message "clicked" should be logged to the console. With an id it works:

<!DOCTYPE html>
<html lang="en">
<head>
    <script defer src="https://pyscript.net/alpha/pyscript.js"></script>
</head>

<body>
	<py-script>
from js import console
def clicked(*args, **kwargs):
	console.log("clicked")
	</py-script>
	
	<button id="click-button" pys-onClick="clicked">Click me!</button>
	
</body>
</html>

EDIT: Added chrome, firefox version number

The text was updated successfully, but these errors were encountered:

yannickfunk · 2022-05-13T14:41:42Z

I guess this:

pyscript/pyscriptjs/src/components/pyscript.ts

Line 139 in a3e86f3

source = `Element("${el.id}").element.onclick = ${handlerCode}`;

is the related code, it uses the id to create an Element() and pass it to pyodide.runPythonAsync(source). However, there should be another possibility than the Id to create this Element

pnhearer · 2022-05-13T15:23:01Z

I guess this:

pyscript/pyscriptjs/src/components/pyscript.ts

Line 139 in a3e86f3

source = `Element("${el.id}").element.onclick = ${handlerCode}`;

is the related code, it uses the id to create an Element() and pass it to pyodide.runPythonAsync(source). However, there should be another possibility than the Id to create this Element

Looks right. Not sure by what other mechanism you can reference the element though other than maybe 'self' if no element id is specified?

pauleveritt · 2022-05-13T21:43:16Z

I'm actually getting this with hello_world.html from a checkout. The checkout worked a few days ago. I can't spot any changes in the commits that might have caused it. Still pointing to 0.20 of Pyodide.

pauleveritt · 2022-05-13T23:06:25Z

As it turns out, I get the exception in Chrome, not in Firefox or Safari:

JsException: SyntaxError: Failed to execute 'querySelector' on 'Document': '#-509153c5-29df-0d1d-f9fd-c7c688f46911' is not a valid selector.

As it turns out, I believe that is indeed an invalid selector. A dash can't be followed by a number.

nmstoker · 2022-05-13T23:17:28Z

I'm seeing as well: in Chrome with the basic hello world from Getting Started.

I only get it intermittently - I guess that when i'm refreshing and that GUID type Id happens to have a dash followed by a number (as @pauleveritt mentions) then it fails and other times when I'm lucky enough not to get that it succeeds

pauleveritt · 2022-05-13T23:17:45Z

(Sorry for the noise.) A little bit more info. I was using the examples from a build directory generated by the build npm script. If I use dev, then the above has #PyScript-50 etc.

pauleveritt · 2022-05-13T23:26:42Z

@nmstoker It appears Element.__init__() is passed a different element_id based on whether you run from a production build or the dev server. The latter is prefixed (correctly) with PyScript. The former is an empty string.

Am I looking at it correctly?

yannickfunk · 2022-05-14T14:16:02Z

As it turns out, I get the exception in Chrome, not in Firefox or Safari:

JsException: SyntaxError: Failed to execute 'querySelector' on 'Document': '#-509153c5-29df-0d1d-f9fd-c7c688f46911' is not a valid selector.

As it turns out, I believe that is indeed an invalid selector. A dash can't be followed by a number.

But I think this is a different error than mine, here '-509153c5-29df-0d1d-f9fd-c7c688f46911' appears to be the element's id. In my case there is no id at all

yannickfunk · 2022-05-14T14:25:44Z

I can also reproduce the issue using firefox 100.0 (64-bit)

yannickfunk · 2022-05-14T15:12:16Z

I can also reproduce with a locally built pyscript.js

yannickfunk · 2022-05-14T18:06:57Z

One fix would be to actually pass the element to python in the initHandlers() like this:

const handlerCode = el.getAttribute('pys-onClick');
const elementGlobals: Map<string, any> = pyodide.globals.toJs()
elementGlobals.set("element", el)
const options = {globals: pyodide.toPy(elementGlobals)};

source = `Element("${el.id}", element).element.onclick = ${handlerCode}`;
output = await pyodide.runPythonAsync(source, options);

This way, Element.element

@property
def element(self):
    """Return the dom element"""
    if not self._element:
        self._element = document.querySelector(f"#{self._id}")
    return self._element

doesn't fail, because the python Element has the actual dom element attached

ptmcg · 2022-05-16T00:56:35Z

I am seeing this even without a button or onClick method - should I open a separate ticket?

Reproduce by viewing this page: http://ptmcg.com/dev/pyscript/pyparsing_railroad_diagram.html

yannickfunk · 2022-05-16T07:48:04Z

@ptmcg It is already discussed here: #370

fpliger · 2022-05-16T16:09:55Z

@yannickfunk can you try now? #395 should have temporarily fixed it (although we need to investigate to actually come with a better fix)

yannickfunk · 2022-05-16T16:14:46Z

@fpliger The error described by me here initially, is NOT the same error as in #370 and as expected is not fixed by #395

fpliger · 2022-05-16T16:16:52Z

Ok, let me test it locally.

yannickfunk · 2022-05-16T16:17:53Z

I also tested it with a local unminified build on chrome and firefox

fpliger · 2022-05-16T16:21:23Z

Ah! Yeah.. Not the same issue. I think problem here is that we are not properly mapping the <py-script> when we generate the output div

TY!

yannickfunk · 2022-05-16T16:27:53Z

I guess this:

pyscript/pyscriptjs/src/components/pyscript.ts

Line 139 in a3e86f3

source = `Element("${el.id}").element.onclick = ${handlerCode}`;

is the related code, it uses the id to create an Element() and pass it to pyodide.runPythonAsync(source). However, there should be another possibility than the Id to create this Element

@fpliger I think the problem is the mapping which happens there. It uses the id of the dom Element, but the in this case has no Id.

One fix would be to actually pass the element to python in the initHandlers() like this:

const handlerCode = el.getAttribute('pys-onClick');
const elementGlobals: Map<string, any> = pyodide.globals.toJs()
elementGlobals.set("element", el)
const options = {globals: pyodide.toPy(elementGlobals)};

source = `Element("${el.id}", element).element.onclick = ${handlerCode}`;
output = await pyodide.runPythonAsync(source, options);

This way, Element.element

@property
def element(self):
    """Return the dom element"""
    if not self._element:
        self._element = document.querySelector(f"#{self._id}")
    return self._element

doesn't fail, because the python Element has the actual dom element attached

With this fix, it works, let me know if this might be an appropriate solution

pauleveritt · 2022-05-16T16:28:07Z

@fpliger @yannickfunk Fixed it for me.

fpliger · 2022-05-16T16:55:28Z

Nice! @yannickfunk that's exactly what's happening. The initially easy "fix" about this is to add an id to the <py-button> component.

In the long term, I think it'd make sense for us to enforce id on every element you add PyScript magic. It is a good practice anyway that we should encourage and makes a lot of the PyScript easier and more explicit. But this is an important decision and probably needs some thinking. Worth opening a dedicated discussion.

yannickfunk · 2022-05-16T16:59:29Z

@fpliger Yeah with an id it works, I already found that out as described in Expected Behavior.
I didn't use a <py-button> but a normal <button> here, does that make a difference?

yannickfunk · 2022-05-16T17:09:30Z

@fpliger Ok so I just found out it actually makes a difference.
For <py-button> the easiest would be to dynamically add an id as for the <pyscript> element (with the checkID() method):

pyscript/pyscriptjs/src/components/pyscript.ts

Line 42 in ca909b4

this.checkId();

For <button> it is the story I described earlier

fpliger · 2022-05-16T17:10:53Z

Not right now but I think that in the long term we can definitely generate an id for a <py-button> component, like we do for <py-script>. It'd be a document expected behaviour for a PyScript component. I'd not really love doing that for <button> elements... I think :)

fpliger · 2022-05-16T17:12:09Z

@fpliger Ok so I just found out it actually makes a difference.
For the easiest would be to dynamically add an id as for the element (with the checkID() method):

We are answering at the same time :)

Right. That's very easy to do but I don't think we have it in <py-button> yet. That's what I meant with the previous comment

yannickfunk · 2022-05-16T17:13:21Z

Yeah for <button> elements we could just pass the dom Element at python Element() creation, if there is no id provided:

One fix would be to actually pass the element to python in the initHandlers() like this:

const handlerCode = el.getAttribute('pys-onClick');
const elementGlobals: Map<string, any> = pyodide.globals.toJs()
elementGlobals.set("element", el)
const options = {globals: pyodide.toPy(elementGlobals)};

source = `Element("${el.id}", element).element.onclick = ${handlerCode}`;
output = await pyodide.runPythonAsync(source, options);

This way, Element.element

@property
def element(self):
    """Return the dom element"""
    if not self._element:
        self._element = document.querySelector(f"#{self._id}")
    return self._element

doesn't fail, because the python Element has the actual dom element attached

fpliger · 2022-05-16T17:28:07Z

So, I think a good fix here would be:

add checkId in PyButton, just like we do in PyScript
add a check when we init the handlers and not allow users to use pys-onClick on elements without id (this should raise an exception

we should also make sure this is clearly stated in the docs (as soon as we merge them this week)

yannickfunk · 2022-05-16T17:29:10Z

@fpliger Sounds good, I could implement this if you want

fpliger · 2022-05-16T18:22:44Z

That'd be great @yannickfunk ! @marimeireles was maybe going to look at this but if she didn't, it'd definitely be a great way to help! TY!

fpliger · 2022-05-16T18:33:31Z

Ok... I don't think @marimeireles started on it yet, @yannickfunk want to pick it up? :)

yannickfunk · 2022-05-16T18:38:09Z

Yeah I would pick it up @fpliger

yannickfunk · 2022-05-16T18:54:22Z

Currently, for the components: The state is the following:

Component	Has `checkId()`
PyBox	[ ]
PyButton	[ ]
PyConfig	[ ]
PyEnv	[ ]
PyInputBox	[ ]
PyLoader	[ ]
PyRepl	[x]
PyScript	[x]
PyTitle	[ ]

Do we want to add it to all components where it is missing, or just to PyButton so far?

fpliger · 2022-05-16T19:18:46Z

Great question. I think it definitely makes sense for the components that can execute a script and are likely to be mounted. (PyConfig and PyEnv are a bit special and I don't think it's necessary). With that mind, that list above should probably look like this:

Component	Has `checkId()`
PyBox	[ ]
PyButton	[x ]
PyConfig	[ ]
PyEnv	[ ]
PyInputBox	[x]
PyLoader	[ ]
PyRepl	[x]
PyScript	[x]
PyTitle	[ ]

yannickfunk · 2022-05-16T20:39:18Z

@fpliger I created a PR

fpliger · 2022-05-16T21:07:02Z

Awesome! I think #400 closes this (tested locally and looks great). TY again @yannickfunk 🚀

Leaving this for posterity... The same example couple have been achieved with the more "PyScripty" form:

<body>
    <py-button id="click_btn" label="Click me!">
        def on_click(evt):
            console.log("clicked")
    </py-button>
</body>

yannickfunk added the needs-triage Is 8000 sue needs triage label May 13, 2022

pauleveritt mentioned this issue May 14, 2022

HTML not getting rendered everytime. #370

Closed

marimeireles added type: bug Something isn't working and removed needs-triage Issue needs triage labels May 16, 2022

fpliger added the tag: component Related to PyScript components label May 16, 2022

fpliger added lang: typescript Related to the Typescript language good first issue Good for newcomers labels May 16, 2022

fpliger assigned marimeireles May 16, 2022

yannickfunk mentioned this issue May 16, 2022

Add checkId() for PyButton and PyInputBox, add id check for tags with pys- Event handlers #400

Merged

fpliger closed this as completed in #400 May 16, 2022

fpliger assigned yannickfunk and unassigned marimeireles May 16, 2022

[BUG] pys-onClick attribute does not work, when element has no id #359

[BUG] pys-onClick attribute does not work, when element has no id #359

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!