Cover the streaming render path, which was never broken - #495
Merged
Conversation
I said in #493 that the streaming path was reasoned about rather than measured, because looking for its canvases found none: document.querySelector("#multitrack-container").querySelectorAll("canvas") // 0 after eight seconds, with the overlay cleared, canplay fired and no failed requests. That was convincing and wrong. WaveSurfer renders into shadow roots and querySelectorAll does not cross a shadow boundary. Walking the roots finds nine canvases, correctly sized, and a zoom re-renders them: 910px each at 1x, and at 5x a 4550px lane cut into 4000 + 550, backing store matching CSS width in every case. So there was nothing to fix, and the gap was mine. The path is covered now, and the assertion is the one that matters on it: WaveSurfer bars are configured in pixels, so they keep their width across a zoom only if it re-renders rather than letting a fixed-size canvas stretch. Backing width equal to CSS width is what distinguishes the two, and a stretched canvas fails it. The shadow-root walk lives in a helper with the trap written down next to it, because the next person to look for a canvas here will otherwise reach the same false conclusion. Closes #494
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #494
I claimed in #493 that the streaming render path was reasoned about rather than
measured, because looking for its canvases found none. That claim was wrong, and
this corrects it.
The false negative
Zero after eight seconds, with the loading overlay cleared,
canplayfired, nofailed requests, and the same result against
main. Every signal said thestreaming path renders nothing.
It renders nine canvases. WaveSurfer draws into shadow roots, and
querySelectorAlldoes not cross a shadow boundary. Walking the roots finds themall, correctly sized.
That is worth more than the coverage it cost: the obvious way to look at this
path returns a convincing wrong answer, so the first person to check concludes it
is broken. The helper that walks the roots carries that warning next to it.
What the test asserts
The zoom promise rests on a different mechanism here than on the SVG path.
WaveSurfer bars are configured in pixels (
barWidth 3,barGap 2), so they keeptheir width across a zoom only if it re-renders rather than letting a
fixed-size canvas stretch.
Backing store width against CSS width is what separates the two, and it is now
measured:
WaveSurfer chunks at 4000 px, which is why a zoomed lane is two canvases rather
than one wide one. A stretched canvas would keep its 910 px backing store while
its box grew, and fails the assertion.
Verification
12 zoom tests pass, 78 browser tests overall. No production code changed: this is
coverage for behaviour that was already correct.