fix(no-autoplay-audio): don't timeout for preload=none media elements #4684
Conversation
There was a problem hiding this comment.
should this PR be marked as a breaking change? Since point 3 in the ticket suggests that it should
| if ( | ||
| actualNode.preload === 'none' && | ||
| actualNode.networkState !== actualNode.NETWORK_LOADING |
There was a problem hiding this comment.
should we ignore the preload="metadata" too? it seems like it doesn't put the media in a ready to play state either
There was a problem hiding this comment.
We don't need to wait for the media to be playable, we just need the metadata so we can identify the duration (that's why isMediaElementReady waits for any non-zero ready state, rather than specifically waiting for a contentful ready state)
| }); | ||
| it('returns false when <video> can autoplay and has no controls mechanism', async () => { | ||
| const checkArgs = checkSetup(` | ||
| <video id="target" autoplay="true"> |
There was a problem hiding this comment.
why are we explicitly specifying the value of the autoplay? it's it just a boolean attribute? E.g. autoplay = "false" is equal to autoplay="true" and will still autoplay.
| '<source src="/test/assets/video.mp4" type="video/mp4" />' + | ||
| '</video>', | ||
| { allowedDuration: 15 } | ||
| it('returns false when <video> loops and has no controls mechanism when duration is unknown', () => { |
There was a problem hiding this comment.
Is the duration actually unknown for this (and all the other test cases meant to exercise unknown duration paths?) It looks like this is using the same media sources as the other test cases for known durations
| } | ||
|
|
||
| /** | ||
| * Ignore media nodes which are `paused` or `muted` |
There was a problem hiding this comment.
Can this comment explain why it's doing this, as opposed to what it's doing? The what is already clear from the code.
| * Ignore media nodes which are `paused` or `muted` | ||
| */ | ||
| if ( | ||
| actualNode.hasAttribute('paused') || |
There was a problem hiding this comment.
no-autoplay-audio also omits cases without an autoplay attribute; did you leave out that case intentionally?
There was a problem hiding this comment.
I'm just realizing now on a fresh reading of the spec that if we do restrict to only waiting for autoplay cases, we actually might want to not add the new logic for preload="none" because of this note in the spec:
Note: The autoplay attribute can override the preload attribute (since if the media plays, it naturally has to buffer first, regardless of the hint given by the preload attribute). Including both is not an error, however.
| if ( | ||
| actualNode.preload === 'none' && | ||
| actualNode.networkState !== actualNode.NETWORK_LOADING |
There was a problem hiding this comment.
We don't need to wait for the media to be playable, we just need the metadata so we can identify the duration (that's why isMediaElementReady waits for any non-zero ready state, rather than specifically waiting for a contentful ready state)
Closes: #4665