Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Frontend/ui-library/src/Overlay/AFKOverlay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ export class AFKOverlay extends ActionOverlay {
* @param countdown - the count down number to be inserted into the span for updating
*/
public updateCountdown(countdown: number): void {
this.textElement.innerHTML = `<center>No activity detected<br>Disconnecting in <span id="afkCountDownNumber">${countdown}</span> seconds<br>Click to continue<br></center>`;
const countDownSpan = this.textElement.querySelector('#afkCountDownNumber') as HTMLSpanElement | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Selecting the afkCountDownNumber by its id is problematic as we potentially allow for multiple Pixel Streaming instances on the same page.

Instead, it would be better if the afkCountDownNumber element is actually constructed programmatically and the constructed element was stored as a field in this class. That way no query is required to retrieve it, the field, if it exists, can simply be accessed in this function and have its textContent updated accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

The change replaces the previous full innerHTML assignment with a targeted DOM update:

  • Instead of recreating the entire overlay markup on every tick, we query for the existing and update its textContent. This:

    • Prevents re-creating DOM nodes (so attached event listeners or state on child elements aren’t lost),
    • Avoids flicker/layout churn and is more efficient,
    • Eliminates an unnecessary innerHTML write (reducing XSS surface compared to injecting HTML),
    • Gracefully no-ops if the span is not found (we intentionally check for null instead of throwing).
  • Type-wise I cast the result as HTMLSpanElement | null to keep TypeScript happy while allowing the null check.

I tested this manually (overlay shows, countdown value updates each second, click-to-continue still works) and saw no regressions. If you prefer, I can:

  • Use document.getElementById('afkCountDownNumber') for an even clearer lookup, or
  • Add a small unit/DOM test to assert the span's textContent is updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @odaysec,

What Luke's stating is that querying DOM elements by id or class is problematic. This includes document.getElementById().

As he states in his original comment, there could be potentially multiple instances of a PixelStreamingPlayer per web page (see /stresstest.html for an example) which means that all elements must be queried and updated programmatically without querying selectors.

if (countDownSpan) {
countDownSpan.textContent = String(countdown);
}
}
}
35 changes: 26 additions & 9 deletions Frontend/ui-library/src/UI/DataChannelLatencyTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,34 @@
* Check we have results, NaN would mean that UE version we talk to doesn't support our test
*/
if (isNaN(result.dataChannelRtt)) {
this.latencyTestResultsElement.innerHTML = '<div>Not supported</div>';
// Clear any previous content and show a simple "Not supported" message.
this.latencyTestResultsElement.textContent = '';
const notSupportedDiv = document.createElement('div');
notSupportedDiv.textContent = 'Not supported';
this.latencyTestResultsElement.appendChild(notSupportedDiv);
return;
}
let latencyStatsInnerHTML = '';
latencyStatsInnerHTML += '<div>Data channel RTT (ms): ' + result.dataChannelRtt + '</div>';

// Clear previous results before showing new ones.
this.latencyTestResultsElement.textContent = '';

const rttDiv = document.createElement('div');
rttDiv.textContent = 'Data channel RTT (ms): ' + result.dataChannelRtt;
this.latencyTestResultsElement.appendChild(rttDiv);
/**
* Separate path time discovery works only when UE and Player clocks have been synchronized.
*/
if (result.playerToStreamerTime >= 0 && result.streamerToPlayerTime >= 0) {
latencyStatsInnerHTML +=
'<div>Player to Streamer path (ms): ' + result.playerToStreamerTime + '</div>';
latencyStatsInnerHTML +=
'<div>Streamer to Player path (ms): ' + result.streamerToPlayerTime + '</div>';
const playerToStreamerDiv = document.createElement('div');
playerToStreamerDiv.textContent =

Check failure on line 95 in Frontend/ui-library/src/UI/DataChannelLatencyTest.ts

View workflow job for this annotation

GitHub Actions / build-using-local-deps

Delete `⏎···············`
'Player to Streamer path (ms): ' + result.playerToStreamerTime;
this.latencyTestResultsElement.appendChild(playerToStreamerDiv);

const streamerToPlayerDiv = document.createElement('div');
streamerToPlayerDiv.textContent =

Check failure on line 100 in Frontend/ui-library/src/UI/DataChannelLatencyTest.ts

View workflow job for this annotation

GitHub Actions / build-using-local-deps

Delete `⏎···············`
'Streamer to Player path (ms): ' + result.streamerToPlayerTime;
this.latencyTestResultsElement.appendChild(streamerToPlayerDiv);
}
this.latencyTestResultsElement.innerHTML = latencyStatsInnerHTML;
//setup button to download the detailed results
const downloadButton: HTMLInputElement = document.createElement('input');
downloadButton.type = 'button';
Expand All @@ -111,6 +124,10 @@
}

public handleTestStart() {
this.latencyTestResultsElement.innerHTML = '<div>Test in progress</div>';
// Clear any previous results and show a "Test in progress" message.
this.latencyTestResultsElement.textContent = '';
const inProgressDiv = document.createElement('div');
inProgressDiv.textContent = 'Test in progress';
this.latencyTestResultsElement.appendChild(inProgressDiv);
}
}
52 changes: 31 additions & 21 deletions Frontend/ui-library/src/UI/LatencyTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,50 +70,60 @@
*/
public handleTestResult(latencyTimings: LatencyTestResults) {
Logger.Info(JSON.stringify(latencyTimings));
let latencyStatsInnerHTML = '';

// Clear any previous results
const resultsElement = this.latencyTestResultsElement;
resultsElement.innerHTML = '';

if (latencyTimings.networkLatency !== undefined && latencyTimings.networkLatency > 0) {
latencyStatsInnerHTML += '<div>Net latency RTT (ms): ' + latencyTimings.networkLatency + '</div>';
const div = document.createElement('div');
div.textContent = 'Net latency RTT (ms): ' + latencyTimings.networkLatency;
resultsElement.appendChild(div);
}

if (latencyTimings.EncodeMs !== undefined && latencyTimings.EncodeMs > 0) {
latencyStatsInnerHTML += '<div>UE Encode (ms): ' + latencyTimings.EncodeMs + '</div>';
const div = document.createElement('div');
div.textContent = 'UE Encode (ms): ' + latencyTimings.EncodeMs;
resultsElement.appendChild(div);
}

if (latencyTimings.CaptureToSendMs !== undefined && latencyTimings.CaptureToSendMs > 0) {
latencyStatsInnerHTML += '<div>UE Capture (ms): ' + latencyTimings.CaptureToSendMs + '</div>';
const div = document.createElement('div');
div.textContent = 'UE Capture (ms): ' + latencyTimings.CaptureToSendMs;
resultsElement.appendChild(div);
}

if (latencyTimings.browserSendLatency !== undefined && latencyTimings.browserSendLatency > 0) {
latencyStatsInnerHTML +=
'<div>Browser send latency (ms): ' + latencyTimings.browserSendLatency + '</div>';
const div = document.createElement('div');
div.textContent = 'Browser send latency (ms): ' + latencyTimings.browserSendLatency;
resultsElement.appendChild(div);
}

if (
latencyTimings.frameDisplayDeltaTimeMs !== undefined &&
latencyTimings.browserReceiptTimeMs !== undefined
) {
latencyStatsInnerHTML +=
latencyTimings.frameDisplayDeltaTimeMs && latencyTimings.browserReceiptTimeMs
? '<div>Browser receive latency (ms): ' +
latencyTimings.frameDisplayDeltaTimeMs +
'</div>'
: '';
if (latencyTimings.frameDisplayDeltaTimeMs && latencyTimings.browserReceiptTimeMs) {
const div = document.createElement('div');
div.textContent =

Check failure on line 108 in Frontend/ui-library/src/UI/LatencyTest.ts

View workflow job for this annotation

GitHub Actions / build-using-local-deps

Delete `⏎···················`
'Browser receive latency (ms): ' + latencyTimings.frameDisplayDeltaTimeMs;
resultsElement.appendChild(div);
}
}

if (latencyTimings.latencyExcludingDecode !== undefined) {
latencyStatsInnerHTML +=
'<div>Total latency (excluding browser) (ms): ' +
latencyTimings.latencyExcludingDecode +
'</div>';
const div = document.createElement('div');
div.textContent =
'Total latency (excluding browser) (ms): ' + latencyTimings.latencyExcludingDecode;
resultsElement.appendChild(div);
}

if (latencyTimings.endToEndLatency !== undefined) {
latencyStatsInnerHTML += latencyTimings.endToEndLatency
? '<div>Total latency (ms): ' + latencyTimings.endToEndLatency + '</div>'
: '';
if (latencyTimings.endToEndLatency) {
const div = document.createElement('div');
div.textContent = 'Total latency (ms): ' + latencyTimings.endToEndLatency;
resultsElement.appendChild(div);
}
}

this.latencyTestResultsElement.innerHTML = latencyStatsInnerHTML;
}
}
Loading