Closed gederer closed 11 months ago
thanks @gederer !
The function would probably have to stay synchronous (so it wouldn't lose the context) the problem is if it's a sync function... we should also prevent referring to anything other than the event itself in that function (maybe a lint rule and / or throwing an error if anyone tries to read from the lexical scope or something)
pasting here as well because we want to keep this issue opened -
A workaround is to use something like @mhevery suggested -
const anchorElement = useSignal<Element>();
useVisibleTask$(() => {
const handler = (event: Event) => {
event.preventDefault();
// do something here
};
anchorElement.value!.addEventListener('click', handler);
return () => anchorElement.value!.removeEventListener('click', handler);
});
which works well for now
Hey together,
in my view, there is also an issue with useOnWindow
and preventDefault
.
The example below demonstrates a simple version of a Dialog-Component using HTMLDialgo
.
By default the HTMLDialog closes when ESC is pressed on the keyboard.
Dialog-Component listens to keyboard-Events and uses preventDefault
to avoid closing the HTMLDialog.
But preventDefault
takes no effect the first time the ESC is pressed.
After Reopening the Dialog-Compoent again and pressing ESC, preventDefault
is respected and the HTMLDialog stays open
-ā” Demo
Steps for reproduction
Maybe it's worth opening another issue for that? š¤
cc @gederer, @shairez
This behavior could be due to a maybeThen()
call somewhere that will call the function synchronously if it's already loaded.
So by calling .resolve()
on the QRL passed to useWindow
at document load time, it might work as soon as the JS loaded.
Right, it's because of maybeThen. Open this playground and open devtools, then open the dialog and press escape.
This will pause execution inside the useOnWindow. Look at the call stack, there's an async break. Continue.
Now open the dialog again and press escape again. Now the call stack does not have an async break, and the original event is available.
So... Maybe we can support eager loading of select handlers so that custom preventX behavior can be done? Of course, if the event fires before the js loaded it will still not work, so it's not a 100% solution. For that, the JS would need to be part of the initial HTML.
(So the event call happens in https://github.com/BuilderIO/qwik/blob/main/packages/qwik/src/qwikloader.ts#L61C23-L61C23 which is inside an async function and behind an await, but since it's all in the first part of async functions, the call happens synchronously)
I tried to move the handler out and then call resolve manually when the dialog opens, but the QRL actually differs between the exported function and the listener that is added via useOnWindow (playground). So while this would work, it would need support from the framework.
How about an eagerness
option like for useTask$
? With values fire
(default), idle
, load
, visible
?
@wmertens so we discussed this with @manucorporat . They way we would like to solve this is by attaching "pure" functions that can compute whether or not to preventdefault and execute them synchronously.
<script q:func="qwik/json">document.currentScript.qFuncs=[(p0,)=>({"header-open":p0.headerMenuOpen,"menu-open":p0.sideMenuOpen}),
(p0,)=>(p0.theme==="light"?"Dark":"Light"),
(p0,)=>(p0.theme),
(p0,)=>({docsearch:true,"ai-result-open":p0.value}),
(p0,)=>(p0.url.href)
]</script>
The above functions are already serialized by the system as part of the signals. We could use the same trick to attach to <button preventdefault:click="3">
This will attach function 3 to this handler. Because the function is already present, it can be run synchronously and can correctly determine if the event should be prevented.
The restriction is that this function must be pure, but it can access the DOM element to look at additional attributes to determine what it should do.
The way I imagine this is:
component$(() => {
return <button preventdefault:click={(e) => e.key === 'r' }>click</button>;
})
So in the above case, the optimizer would extract the (e) => e.key === 'r'
and add it to <script q:func="qwik/json">
. Obviously this can only be done if the function is pure. So we would also need linter and some good msg from optimizer if it is not.
NOTE: Optimizer is already doing this for signals, so this should not be a large change.
Yes that seems like a nice solution, backwards compatible too
Any update on this? I'm currently refactoring the visible tasks out of Qwik UI's accordion.
can the qwik-ui team get this fixed as a Christmas gift š that way all the useVisableTask can be removed
sharing for others: confirmed on discord that @mhevery is starting to implement his proposal from above š
update from @mhevery on today's office hours call that the upcoming API has been changed a bit to add an event handler wrapped with $sync
, rather than allowing a function to be assigned to preventdefault:click
sync functions will be highly restricted, including (IIUC) not allowing access to component props
I've been looking into how to integrate Qwik and Zag (framework-agnostic lib with state-machine based abstractions of common UI components): https://github.com/chakra-ui/zag/discussions/392 with the eventual goal of getting Qwik support in Ark and Park
Unfortunately neither of the solutions mentioned in this thread (useVisibleTask
or @mhevery 's work on sync()
) seem like they make this integration possible. More details in that discussion on the Zag repo.
Beyond this specific integration, though, this also raises the broader question of whether the sync()
restrictions on being imported (ie that they must be directly written in-line) and not having any access to component state are reasonable/workable in real-world scenarios.
Currently every one of the Zag event handlers that calls preventDefault
accesses some local state (the zag state machine): https://github.com/search?q=repo%3Achakra-ui%2Fzag+preventDefault+path%3Apackages%2Fmachines&type=code
It's plausible that all these event handlers could be split into two:
In many cases the preventDefault call is unconditional, or logic of whether to call preventDefault depends purely on the event, and so this would work. But in a significant number, the preventDefault decision does actually depend on either state (eg radio group & checkbox or the state machine's "context" (eg tags input & dialog)
These seem like pretty valid use-cases, but it's not clear to me how/whether this could be in Qwik. This makes me wonder if there is perhaps a more complete/flexible escape hatch needed that would do a bit more work up-front in order to give (say read-only?) access to more data when operating synchronously
If this were to be made possible, I do understand that there would be temptation to abuse it. I wonder if putting it behind a more obviously-unfriendly name than the current sync
would help? something like PerfKillinglySynchronous()
š
In terms of how this would work, seems @wmertens 's suggestion of somehow allowing direct tweaking of an eagerness option for just loading some QRLs at page load time would be a fairly straightforward (and hopefully easily understood?) solution that should solve this use-case. Discussed across a few messages, starting here. Ideally these could all be optimized into a single bundle (maybe even in-lined in the initial page load?), mitigating the issue of the split-second delay of the handler not working before the QRL is resolved.
Curious what considerations were brought up in this conversation between @mhevery and @manucorporat that resulted in taking a different direction?
Currently every one of the Zag event handlers that calls
preventDefault
accesses some local state (the zag state machine): https://github.com/search?q=repo%3Achakra-ui%2Fzag+preventDefault+path%3Apackages%2Fmachines&type=code
So the idea is two-fold (yes it requires thinking about the problem differently)
So when the sync function wakes up, it either has all of the information it needs to process the event, OR it relies on the rest of the codebase to data bind relevant state as an attribute.
Could you come up with a use case that can't be solved in this way?
I've been looking into how to integrate Qwik and Zag (framework-agnostic lib with state-machine based abstractions of common UI components): chakra-ui/zag#392
Unfortunately neither of the solutions mentioned in this thread (
useVisibleTask
or @mhevery 's [work onsync()
[(#5545) ) seem like they make this integration possible. More details in that discussion.
@gabrielgrant it's awesome that you've been looking into this. Having another resumable UI library alternative would be a game changer.
Why exactly does prevent default behavior not work in useVisibleTask$
? In Qwik UI, this has been the workaround that we've used.
Beyond this specific integration, though, this also raises the broader question of whether the
sync()
restrictions on being imported (ie that they must be directly written in-line) and not having any access to component state are reasonable/workable in real-world scenarios.Currently every one of the Zag event handlers that calls
preventDefault
accesses some local state (the zag state machine): https://github.com/search?q=repo%3Achakra-ui%2Fzag+preventDefault+path%3Apackages%2Fmachines&type=codeIt's plausible that all these event handlers could be split into two:
- a sync function that conditionally calls preventDefault
- an async function that transitions the state machine
In many cases the preventDefault call is unconditional, or logic of whether to call preventDefault depends purely on the event, and so this would work. But in a significant number, the preventDefault decision does actually depend on either state (eg radio group & checkbox or the state machine's "context" (eg tags input & dialog)
Yeah this does seem like something specific to state machines. Which I don't know much about š . Is this not possible?
// inside whatever event we have
onFocus$(e)={
// some work here
$sync((e) => {
if (e.key === 'downArrow') {
e.preventDefault()
}
}))
}
somewhere else in the code:
// some logic I need to perform async with state
I haven't tried $sync
yet, and so may come across this use case, not sure.
These seem like pretty valid use-cases, but it's not clear to me how/whether this could be in Qwik. This makes me wonder if there is perhaps a more complete/flexible escape hatch needed that would do a bit more work up-front in order to give (say read-only?) access to more data when operating synchronously
If this were to be made possible, I do understand that there would be temptation to abuse it. I wonder if putting it behind a more obviously-unfriendly name than the current
sync
would help? something likePerfKillinglySynchronous()
š
My understand is that this is exactly what $sync
is. A special syntax that gives us read-only access to do things synchronously.
@thejackshelton thanks for taking a look!
Having another resumable UI library alternative would be a game changer.
Yea, mentioned this on the office hours call a couple weeks ago -- I love what you guys are doing with qwik UI to figure out the best qwik-native ways of doing things, but I also think being able to integrate with the tools that the rest of the front end world uses is super important too.
Why exactly does prevent default behavior not work in useVisibleTask$? In Qwik UI, this has been the workaround that we've used.
A couple reasons:
preventDefault()
My understand is that this is exactly what $sync is. A special syntax that gives us read-only access to do things synchronously.
The current implementation gives access to the event and DOM, but no access (not even read-only) to component state, which they use in a good number of the more complex components when deciding whether to preventDefault()
For example their dialog component allows choosing whether the dialog gets closed on ESC using if (!ctx.closeOnEscapeKeyDown) event.preventDefault()
All usages of preventDefault in Zag: https://github.com/search?q=repo%3Achakra-ui%2Fzag+preventDefault+path%3Apackages%2Fmachines&type=code
@mhevery i think i follow -- the idea is that the relevant component state would be manually bound to the element in, eg, data-my-state-x
attrs and then the sync()
-wrapped handler would pull those from the DOM?
That does seem like it should be doable. Feels a bit complex/manual for general usage, but I guess the whole point is to discourage frequent usage unless you really deeply understand what you're doing and know you need it?
The other issue in the case of Zag specifically (or any lib, for that matter), is the restriction against using any imported function (my understanding is that restriction applies even if the imported function is pure?). Is this limitation fundamental for some reason? Or more just that import support was a pain to implement for an initial experiment?
In general, it kinda makes sense that the actual event handler should be right in the component, if the suggested way of passing state to that event handler relies on binding data on the element (splitting them up would be pretty confusing to read). But both those design decisions makes it quite difficult to abstract away synchronous component logic (and esp when trying to deal with a 3rd party lib like Zig).
In terms of making it possible to use Zag in the short-term before pushing major refactorings upstream to split all their handlers, is there any plausible path forward with @wmertens 's suggestion of eagerly loading some QRLs ?
A lot of people got tired of implementing the same thing over and over again in different frameworks, I've considered how to use zag in qwik before, and as far as current solutions go, it's very hard to do that.
so you could in theory, use import('...')
but that would require extra work. and you would have async behavior. Why can't you put zag code in the regular async handler? Would that not work?
@mhevery the Zag event handlers often call preventDefault (and a good number of them make use of component state, not just event info, to decide whether to call preventDefault()). For example their dialog component allows choosing whether the dialog gets closed on ESC using if (!ctx.closeOnEscapeKeyDown) event.preventDefault()
I may not be correctly following what you're asking/suggesting tho?
Maybe the ability to nest $sync
QRL's would fix this problem?
onEscapeKeyDown(event) {
if (!ctx.closeOnEscapeKeyDown) {
$sync(() => event.preventDefault());
}
else send({ type: "CLOSE", src: "escape-key" })
ctx.onEscapeKeyDown?.(event)
},
To be clear: it seems plausible that Zag would perhaps accept all their event handlers being split into a sync and async portion. I'm less confident they'll want to use this via-the-DOM method of passing state around for all other frameworks that don't require the indirection (I imagine this constant DOM manipulation has some negative perf impacts too, no?)
So thinking this approach may work:
normalizeProps
?) to bind those values into the DOMsync()
(via Zag's normalizeProps
?) before they are returned to be spreadThat still relies on sync handlers being able to be imported and spread onto components, though. Is that a possible addition @mhevery ?
If so, that seems like a reasonable long-term path from the Qwik POV, but this would be a significant change to push upstream. So my thinking on how to proceed short-term is the less-optimal method of just resolving the relevant Zig event handlers' QRLs (only those calling preventDefault) on page load
@mhevery I think what you're saying above about import()
is that if I've already imported the chunk that holds the relevant QRL, then there shouldn't be a loading delay when the event triggers?
But, as you point out, even if the chunk has already been imported, the re-import done by resolve()
will still be async, so IIUC preventDefault() still won't work.
It seems like to really make QRL-based handlers work synchronously (and thus preventDefault to work), I need to actually resolve the QRL eagerly (like get a reference to the qrl
and call resolve()
, not just import the module) so that when the event actually fires, we can avoid this asynchronous code path completely
Any thoughts on how to eagerly resolve the QRLs? Pretty sure that's what @wmertens said would need support from the framework but wondering if there may be some kinda hacky way to do it by accessing something in the global/container namespace?
I think if we add an eager option to $, which means it would resolve the QRL on load, all of the zag concerns would be solved.
Once a QRL is resolved, it runs synchronously and preventdefault works.
Yes it means loading js but that's ok when it's useful.
It does leave a short race condition for events happening before it loaded, which can be shortened by putting the loader at the top of the document.
Is your feature request related to a problem?
Currently, it is not possible to implement conditional
preventDefault
behavior in Qwik components. In other frameworks, we would just calle.preventDefault()
within an event handler when needed. But, because of the way Qwik is implemented, this is not possible in Qwik.This poses serious limitations when a component author wants to reuse most of the browser's event handling behavior for a particular event, but needs to override some of this behavior within an event handler.
Describe the solution you'd like
The proposed solution is prevent default functions. This would allow the component author to pass a function into
preventdefault:*
that would returntrue
if the default should be prevented, andfalse
, otherwise.Example:
In the above example, the
keyup
default behavior would be prevented if the user pressed the Enter key, but not if the user pressed any other key.Describe alternatives you've considered
The component author can use the existing
prevent default:*
attributes to prevent all default behavior, then reimplement all of the desired default behavior (as well any custom behavior) in the event handler. This approach is, however, both cumbersome, and not performant.Additional context
This problem has come up a number of times in the qwik-ui project.
Related PRs
4625