polimediaupv / paella-core

Paella Player core library
Educational Community License v2.0
20 stars 15 forks source link

In Chrome, loading problematic videos causes inconsistency in play/pause and volume/mute icons, and timeline text rendering #344

Closed karendolan closed 4 months ago

karendolan commented 10 months ago

Issue: in Chrome browser on MacOSX on video load, in some videos, about 90% of the time when they load, Chrome only renders innerHTML() changes on every other change. In-between, it renders the innerHTML() DOM change as blank or empty. This causes the timeline text to flash as it toggles between rendering text and empty on every timeupdate. This also causes the play/pause button icon to toggle from an icon to empty on every click and the volume mute icon to toggle between empty and icon.

The issue immediately resolves itself when some item in the DOM gets a style change. For example, when changing to fullscreen and back, or hiding the control bar and revealing it again, Chrome begins to render the DOM innerHTML() changes properly.

Our site resolved this issue by showing the showHandler in the progressIndicator configuration of our Paella7 configuration file. The showHandler gets a style change on every timeupdate. This style update nudge solves Chrome's inconsistency in rendering the DOM innerHTML() changes.

The issue might only affect our site because we have problematic videos. The issue does not happen on all our videos, but appears to be on videos that have evidence of HLS.js nudge/error handling during the load.

This only happens in Chrome, not Safari or Firefox. I've only tested on MacOSX lapbook. Chrome hardware acceleration and all Chrome extensions were disabled.

karendolan commented 10 months ago

Screen shots examples of un-rendered but existing DOM innerHTML elements in the control bar:

Screenshot 2023-10-24 at 9 30 17 AM

Example of missing play/pause volume/mute and timeline text

Screenshot 2023-10-24 at 9 30 01 AM

Example of missing play icon toggled to view and volume toggled to view and missing timeline text

Screenshot 2023-10-24 at 9 29 50 AM

Example of missing play/pause icon and timeline text flashing into view on a timeupdate

karendolan commented 9 months ago

FYI, a workaround to this issue was adding a css artifact so that enabling the showHandler in progressIndicator forces Chrome to render innerHtml changes.

This is what we did at our site which we cannot contribute because it is a hack to use a plugin that we are not using as a tool to force a CSS re-render in Chrome.


diff --git a/etc/ui-config/mh_default_org/paella7/config.json b/etc/ui-config/mh_default_org/paella7/config.json
index 6fa1aa751f..fd01497fe7 100644
--- a/etc/ui-config/mh_default_org/paella7/config.json
+++ b/etc/ui-config/mh_default_org/paella7/config.json
@@ -17,7 +17,7 @@
         "showTotal": true,
         "parentContainer": "buttonArea",
         "side": "left",
-        "visible": false,
+        "visible": true,
         "showHandler": true,
         "hideHandlerOnMouseOut": true,
         "showRemainingProgress": true
diff --git a/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.css b/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.css
index 6ab3446866..624227c1ab 100644
--- a/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.css
+++ b/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.css
@@ -162,10 +162,17 @@ ul.menu-button-content li.menu-button-item button span.menu-title {
     height: .7em;
 }

+/* #DCE OPC-931 The progressIndicator showHandler needs be true to workaround the Chrome render race condition issue on timeupdate
+ * in some DCE videos.
+ * Enabling showHandler causes its style attribute toupdate on timeupdate, which causes Chrome to properly render
+ * the innerHTML changes for: icon in the play-pause button, the text in the timeline time area, and volume mute/umute icon.
+ * Setting opacity to 0 here on the handler, to avoid showing the handler artifact on the timeline
+ * */
 .progress-indicator .progress-indicator-handler {
     background-color: white;
-    width: 16px;
-    height: 16px;
+    width: 8px;
+    height: 8px;
+    opacity: 0;
 }

 .progress-indicator .progress-indicator-content {
diff --git a/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.json b/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.json
index 02c22ffa2f..978c3d967b 100644
--- a/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.json
+++ b/etc/ui-config/mh_default_org/paella7/dce_7_theme/theme.json
@@ -4,17 +4,6 @@
         "theme.css"
     ],
     "configOverrides": {
-        "progressIndicator": {
-            "inlineMode": false,
-            "showTotal": true,
-            "parentContainer": "buttonArea",
-            "side": "left",
-            "visible": true,
-            "showHandler": false,
-            "hideHandlerOnMouseOut": true,
-            "showRemainingProgress": true
-        },
-        
         "videoContainer": {
             "overPlaybackBar": false,
ferserc1 commented 9 months ago

Can you send a link to one of these problematic videos?

I don't feel very comfortable adding such a fix, I would prefer to investigate the bug further to see if I can fix it at source, within the plugin itself that handles HLS streams.

karendolan commented 9 months ago

@ferserc1 that sounds great. The theory, because forcing a style change on the progress indicator resolves it, is that the issue is where there is an element.innerHTML change and no companion element.style change. These areas: https://github.com/polimediaupv/paella-core/blob/main/src/js/core/ProgressIndicator.js#L50 https://github.com/polimediaupv/paella-core/blob/main/src/js/core/ProgressIndicatorTimer.js#L12-L28

I will start a test server and undo our hack patch (that force a css change) in order for you to see the issue. I send the link directly to you in an email.

karendolan commented 8 months ago

I will start a test server and undo our hack patch (that force a css change) in order for you to see the issue. I send the link directly to you in an email.

Hi @ferserc1, I could not reproduce the issue after removing our hack style update patch last week. After a lot of research, the issue seems related to Chrome's flurry of fall 2023 security patches around the use of "innerHTML". In summary, there is a lot of overhead, security checks, continual patches around the use of "innerHTML" and it's "parseHTMLUnsafe" sub process.

The take-away for me it that when setting flat text on a node, it is less problematic, costly, and issue prone, to use "innerText" than "innerHTML". https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText

ferserc1 commented 4 months ago

Hi @karendolan.

I have tried to reproduce this problem but have not been able to, so taking into account your last comment, I will close this issue. If necessary it will be reopened, in case we identify the problem.