Changeset 293658 in webkit

Timestamp:

May 1, 2022 6:48:01 PM (3 months ago)

Author:

Devin Rousso

Message:

[Modern Media Controls] the overflow button sometimes flickers
​https://bugs.webkit.org/show_bug.cgi?id=239921
<rdar://problem/91329468>

Reviewed by Eric Carlson.

There are two things that control the visibility of the OverflowButton:

1. whether any of the "default" actions (e.g. playback speed, chapters, etc.) are possible
2. if any other buttons that have contextMenuOptions are dropped (i.e. there's not enough

room for it because the <video> is narrow or there are already too many buttons)

(1) is recalculated for most JS media events (e.g. whenever tracks are changed, if
the readyState changes, etc.).

(2) is recalculated in layout of MediaControls, which is (relatively) less frequent.

In the case that the only contextmenu options are provided by (2) (i.e. none of the "default"
actions are possible), the frequent recalculation of (1) will combined with the fact that
layout uses a requestAnimationFrame to delay/batch work will cause there to be a short
period of time after the recalculation of (1) and before the recalculation of (2) where
there are no contextmenu options, resulting in the OverflowButton being hidden.

• Modules/modern-media-controls/controls/overflow-button.js:

(OverflowButton):
(OverflowButton.prototype.set visible):
(OverflowButton.prototype.set visible.isEmpty): Added.
(OverflowButton.prototype.get contextMenuOptions):
(OverflowButton.prototype.addExtraContextMenuOptions): Renamed from addContextMenuOptions.
(OverflowButton.prototype.clearExtraContextMenuOptions): Renamed from clearContextMenuOptions.
(OverflowButton.prototype.set defaultContextMenuOptions):

• Modules/modern-media-controls/controls/inline-media-controls.js:

(InlineMediaControls.prototype.layout):

• Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js:

(MacOSFullscreenMediaControls.prototype.layout):
Instead of having a single _contextMenuOptions that is modified by both (1) and (2), have
a separate member variable for each. This way, the recalculation of (1) doesn't also clear
the state left over from the last time (2) was calculated (which will be recalculated by (2)
shortly thereafter). Use both member variables to decide whether the OverflowButton should
be visible, allowing (1) and (2) to update independent of eachother.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/modern-media-controls/controls/inline-media-controls.js (modified) (3 diffs)
• Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js (modified) (2 diffs)
• Modules/modern-media-controls/controls/overflow-button.js (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-05-01  Devin Rousso  <drousso@apple.com>
+
+        [Modern Media Controls] the overflow button sometimes flickers
+        https://bugs.webkit.org/show_bug.cgi?id=239921
+        <rdar://problem/91329468>
+
+        Reviewed by Eric Carlson.
+
+        There are two things that control the visibility of the `OverflowButton`:
+        1. whether any of the "default" actions (e.g. playback speed, chapters, etc.) are possible
+        2. if any other buttons that have `contextMenuOptions` are dropped (i.e. there's not enough
+        room for it because the `<video>` is narrow or there are already too many buttons)
+
+        (1) is recalculated for most JS media events (e.g. whenever tracks are changed, if
+        the `readyState` changes, etc.).
+
+        (2) is recalculated in `layout` of `MediaControls`, which is (relatively) less frequent.
+
+        In the case that the only contextmenu options are provided by (2) (i.e. none of the "default"
+        actions are possible), the frequent recalculation of (1) will combined with the fact that
+        `layout` uses a `requestAnimationFrame` to delay/batch work will cause there to be a short
+        period of time after the recalculation of (1) and before the recalculation of (2) where
+        there are no contextmenu options, resulting in the `OverflowButton` being hidden.
+
+        * Modules/modern-media-controls/controls/overflow-button.js:
+        (OverflowButton):
+        (OverflowButton.prototype.set visible):
+        (OverflowButton.prototype.set visible.isEmpty): Added.
+        (OverflowButton.prototype.get contextMenuOptions):
+        (OverflowButton.prototype.addExtraContextMenuOptions): Renamed from `addContextMenuOptions`.
+        (OverflowButton.prototype.clearExtraContextMenuOptions): Renamed from `clearContextMenuOptions`.
+        (OverflowButton.prototype.set defaultContextMenuOptions):
+        * Modules/modern-media-controls/controls/inline-media-controls.js:
+        (InlineMediaControls.prototype.layout):
+        * Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js:
+        (MacOSFullscreenMediaControls.prototype.layout):
+        Instead of having a single `_contextMenuOptions` that is modified by both (1) and (2), have
+        a separate member variable for each. This way, the recalculation of (1) doesn't also clear
+        the state left over from the last time (2) was calculated (which will be recalculated by (2)
+        shortly thereafter). Use both member variables to decide whether the `OverflowButton` should
+        be `visible`, allowing (1) and (2) to update independent of eachother.
+
 2022-04-30  Andres Gonzalez  <andresg_22@apple.com>
 

trunk/Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js

@@ -170 +170 @@
         this.rightContainer.children.concat(this.leftContainer.children).forEach(button => delete button.dropped);
         this.muteButton.style = this.preferredMuteButtonStyle;
-        this.overflowButton.clearContextMenuOptions();
+        this.overflowButton.clearExtraContextMenuOptions();
 
         for (let button of this._droppableButtons()) {
@@ -189 +189 @@
 
             if (button !== this.overflowButton)
-                this.overflowButton.addContextMenuOptions(button.contextMenuOptions);
+                this.overflowButton.addExtraContextMenuOptions(button.contextMenuOptions);
         }
 
@@ -201 +201 @@
 
             button.dropped = true;
-            this.overflowButton.addContextMenuOptions(button.contextMenuOptions);
+            this.overflowButton.addExtraContextMenuOptions(button.contextMenuOptions);
         }
 

trunk/Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js

@@ -115 +115 @@
 
         this._rightContainer.children.forEach(button => delete button.dropped)
-        this.overflowButton.clearContextMenuOptions();
+        this.overflowButton.clearExtraContextMenuOptions();
 
         this._leftContainer.visible = this.muteButton.enabled;
@@ -129 +129 @@
 
             button.dropped = true;
-            this.overflowButton.addContextMenuOptions(button.contextMenuOptions);
+            this.overflowButton.addExtraContextMenuOptions(button.contextMenuOptions);
         }
 

trunk/Source/WebCore/Modules/modern-media-controls/controls/overflow-button.js

@@ -35 +35 @@
         });
 
-        this.clearContextMenuOptions();
         this._defaultContextMenuOptions = {};
+        this._extraContextMenuOptions = {};
     }
 
@@ -48 +48 @@
     set visible(flag)
     {
-        let hasContextMenuOptions = false;
-        for (let key in this._contextMenuOptions) {
-            hasContextMenuOptions = true;
-            break;
+        function isEmpty(contextMenuOptions) {
+            for (let key in contextMenuOptions)
+                return false;
+            return true;
         }
 
-        super.visible = flag && hasContextMenuOptions;
+        super.visible = flag && (!isEmpty(this._defaultContextMenuOptions) || !isEmpty(this._extraContextMenuOptions));
     }
 
     get contextMenuOptions()
     {
-        return this._contextMenuOptions;
+        return {
+            ...this._defaultContextMenuOptions,
+            ...this._extraContextMenuOptions,
+        };
     }
 
-    addContextMenuOptions(contextMenuOptions)
+    addExtraContextMenuOptions(contextMenuOptions)
     {
         if (!this.enabled)
@@ -68 +71 @@
 
         for (let key in contextMenuOptions)
-            this._contextMenuOptions[key] = contextMenuOptions[key];
+            this._extraContextMenuOptions[key] = contextMenuOptions[key];
 
         this.visible = true;
     }
 
-    clearContextMenuOptions()
+    clearExtraContextMenuOptions()
     {
-        this._contextMenuOptions = {};
+        this._extraContextMenuOptions = {};
 
         this.visible = false;
-
-        this.addContextMenuOptions(this._defaultContextMenuOptions);
     }
 
@@ -86 +87 @@
         this._defaultContextMenuOptions = defaultContextMenuOptions || {};
 
-        this.clearContextMenuOptions();
-
-        if (this.layoutDelegate)
-            this.layoutDelegate.needsLayout = true;
+        this.visible = true;
     }