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

fix(qwikloader): stopPropagation for any async handlers #6123

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

PatrickJS
Copy link
Member

@PatrickJS PatrickJS commented Apr 16, 2024

sync$ may not be async function and e.stopPropagation() won't work unless it's fired immediately

fixes #5322

used this repo with the fix in a custom qwik loader
https://github.com/PatrickJS/bug-qwik-loader-stopPropagation

sync$ may not be async function and e.stopPropagation() won't work unless it's fired immediately
@PatrickJS PatrickJS marked this pull request as draft April 16, 2024 18:45
@PatrickJS
Copy link
Member Author

this fix wasn't the fix I'm looking into qwikloader

@PatrickJS
Copy link
Member Author

PatrickJS commented Apr 16, 2024

this might be a browser bug or most likely the cancelBubble only happens because it finishes propagation so after setTimeout the event is reset. we have to run stopPropagation after each async action to maintain the correct state

// Add a click event listener to the document body
document.body.addEventListener('click', function(e) {
  // Stop propagation
  e.stopPropagation();
  
  // Log cancelBubble value immediately after stopping propagation
  console.log('Immediate cancelBubble:', e.cancelBubble); 

  // Set a timeout
  setTimeout(function() {
    // Log cancelBubble value after the timeout
    console.log('After setTimeout cancelBubble:', e.cancelBubble);
    // e.cancelBubble <--- should be true but it's false 
  }, 1000);
});

fix multiple ways to stopPropagation
@PatrickJS PatrickJS marked this pull request as ready for review April 16, 2024 21:50
@PatrickJS PatrickJS changed the title fix(qwikloader.ts): await only async handlers fix(qwikloader): stopPropagation for any async handlers Apr 16, 2024
@PatrickJS PatrickJS force-pushed the fix-qwikloader-sync-handlers branch from a1237da to c19e9ad Compare April 16, 2024 22:01
@PatrickJS PatrickJS requested a review from wmertens April 16, 2024 22:02
@thejackshelton
Copy link
Member

LGTM 🙌 . Merging

@thejackshelton thejackshelton merged commit 03b4b6f into main Apr 16, 2024
24 checks passed
@thejackshelton thejackshelton deleted the fix-qwikloader-sync-handlers branch April 16, 2024 23:13
}
// forcing async with await resets ev.cancelBubble to false
if (cancelBubble) {
ev.stopPropagation();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be called before awaiting?

Copy link
Member

@thejackshelton thejackshelton Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be called before awaiting?

Ah yes, I don't think we want any possible delay here, so something like:

const cancelBubble = ev.cancelBubble;
if (cancelBubble) {
  ev.stopPropagation();
}
if (isPromise(results)) {
  await results;
}

does that look right @wmertens ? If so happy to make another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[✨] stopPropagation:{eventName}
3 participants