QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.81k stars 1.3k forks source link

[šŸž] useVisibleTask$ does not fire on <audio /> element #5142

Closed n8sabes closed 6 months ago

n8sabes commented 1 year ago

Which component is affected?

Qwik Runtime

Describe the bug

useVisibleTask$() should fire on <audio /> elements. It fires with null and everything else, but not audio.

FIRES

import { component$, useVisibleTask$ } from '@builder.io/qwik';

export default component$(() => {
  useVisibleTask$(()=>console.log('fires'))
  return null;
});

DOES NOT FIRE

import { component$, useVisibleTask$ } from '@builder.io/qwik';

export default component$(() => {
  useVisibleTask$(()=>console.log('does not fire'))
  return <audio />;
});

Reproduction

https://qwik.builder.io/playground/#v=1.2.10&f=Q0o0Rham2BKNDnqZRnEqQi8jITLItbU6upVpmUWpKeqaOmD3AyuQalD7AJj80yutFNRToNlJNzMlJ1Ud6DyggvD8ouxEcJUA8Soi7SaWpmTmAwPGGqQMIghqmIDLT5A1xTrA1gGwyAIrS8kHFkB5%2BSWj6XuIpm8A

Steps to reproduce

No response

System Info

System:
    OS: macOS 13.3.1
    CPU: (8) arm64 Apple M1
    Memory: 72.64 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.16.0 - ~/.nvm/versions/node/v18.16.0/bin/node
    npm: 9.5.1 - ~/.nvm/versions/node/v18.16.0/bin/npm
  Browsers:
    Chrome: 116.0.5845.96
    Firefox Developer Edition: 117.0
    Safari: 16.4
  npmPackages:
    @builder.io/qwik: ^1.2.10 => 1.2.10
    @builder.io/qwik-city: ^1.2.10 => 1.2.10
    undici: 5.22.1 => 5.22.1
    vite: 4.4.7 => 4.4.7

Additional Information

WORKAROUND:

Use { strategy: 'document-idle' }

import { component$, useVisibleTask$ } from '@builder.io/qwik';

export default component$(() => {
  useVisibleTask$(() => console.log('useVisibleTask$ fired'),
    { strategy: 'document-idle' }   // Workaround
  );
  return <audio />;
});
mhevery commented 1 year ago

This is probably because <audio/> is not considered a visible element and hence the browser does not fire intersection observer. Not sure much can be done about this.

n8sabes commented 1 year ago

Nor is null, but it fires the task. Working around it is not obvious w/ the { strategy: 'document-idle' }.

Can we make them consistent?

mhevery commented 1 year ago

OK did some testing...

<audio/> has display: none which prevents the intersection observer from running.

The same thing happens with

export default component$(() => {
  useVisibleTask$(() => console.log("Works"));
  return <div style={{ display: "none" }} />;
});

Turns out this does not help:

export default component$(() => {
  useVisibleTask$(() => console.log("Works"));
  return <audio style={{ display: "block" }} />;
});

Even though we say display: block the <audio> is still display: none.

When you return null, what is actually generated is:

<script type="placeholder" hidden="" q:id="8" on-document:qinit="/src/issue_5142_component_usevisibletask_2yrvsynfeuy.js#_hW[0]"></script>

But notice it is no longer visible but qinit (which is document-ready)

So I am not sure what a good solution would look like.

n8sabes commented 1 year ago
  1. To keep the learning curve down, it would be most intuitive to fire when <audio /> is rendered to the DOM. I've not looked at the core code (yet), but if a simple fix is possible that would seem most intuitive.

  2. Otherwise if not desirable, I can add a special note to the Media Player Cookbook Recipe to add { strategy: 'document-idle' } as a workaround for this issue.

??

mhevery commented 1 year ago
  • To keep the learning curve down, it would be most intuitive to fire when <audio /> is rendered to the DOM. I've not looked at the core code (yet), but if a simple fix is possible that would seem most intuitive. Not sure how such a fix could be constructed....

  • Otherwise if not desirable, I can add a special note to the Media Player Cookbook Recipe to add { strategy: 'document-idle' } as a workaround for this issue. Yes, I think that is correct.

Also we should add coveats to the useVisibleTask#() explaining this. Would you be up for PR?

n8sabes commented 1 year ago

I've put it in my ToDo list....

PatrickJS commented 6 months ago

adding to docs to close this

PatrickJS commented 6 months ago

added docs about this here https://github.com/QwikDev/qwik/pull/6255