Bug 163444 – [Modern Media Controls] macOS inline controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Antoine Quint]

This task covers the creation of a class to instantiate the media controls for inline media playback on macOS.

[Antoine Quint]

<rdar://problem/27989473>

[Antoine Quint]

Created attachment 291633 [details]
Patch

[Dean Jackson]

Comment on attachment 291633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291633&action=review

> Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:47
> +        this._leftContainer = new ButtonsContainer({
> +            buttons: [this.playPauseButton, this.skipBackButton],
> +            cssClassName: "left",
> +            padding: 24,
> +            margin: 24
> +        });
> +
> +        this._rightContainer = new ButtonsContainer({
> +            buttons: [this.muteButton, this.airplayButton, this.pipButton, this.tracksButton, this.fullscreenButton],
> +            cssClassName: "right",
> +            padding: 24,
> +            margin: 24
> +        });

Should we be calling these left and right? What about RTL mode?

Maybe call them Playback and Settings, or something like that.

> Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:101
> +        // Reset dropped buttons.
> +        this._rightContainer.buttons.concat(this._leftContainer.buttons).forEach(button => delete button.dropped);
> +
> +        this._leftContainer.layout();
> +        this._rightContainer.layout();
> +
> +        this.timeControl.width = this.width - this._leftContainer.width - this._rightContainer.width;
> +
> +        if (this.timeControl.isSufficientlyWide) {
> +            this.controlsBar.insertBefore(this.timeControl, this._rightContainer);
> +            this.timeControl.x = this._leftContainer.width;
> +        } else {
> +            this.timeControl.remove();
> +            // Since we don't have enough space to display the scrubber, we may also not have
> +            // enough space to display all buttons in the left and right containers, so gradually drop them.
> +            for (let control of [this.airplayButton, this.pipButton, this.tracksButton, this.muteButton, this.skipBackButton, this.fullscreenButton]) {
> +                // Nothing left to do if the combined container widths is shorter than the available width.
> +                if (this._leftContainer.width + this._rightContainer.width < this.width)
> +                    break;
> +
> +                // If the control was already not participating in layout, we can skip it.
> +                if (!control.visible)
> +                    continue;
> +
> +                // This control must now be dropped.
> +                control.dropped = true;
> +
> +                this._leftContainer.layout();
> +                this._rightContainer.layout();
> +            }
> +        }

I don't really understand how this works. We start by deleting all buttons that are dropped.

But I don't understand the "delete button.dropped" line (72). Isn't button.dropped just a boolean?
Did you mean to do .filter(button => button.dropped).forEach(button => delete button)

Then we go on to the loop over the left-side controls. If we need to drop the control, we set dropped to true, and call layout again. Why do you need to layout the left side after this? And won't it trigger this layout function to run again, since the children may have changed sizes? If so, aren't you doing more work than necessary?

> Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:104
> +        this._volumeSliderContainer.x = this._rightContainer.x + this.muteButton.x;

What if muteButton has been dropped?

> LayoutTests/media/modern-media-controls/macos-inline-media-controls/macos-inline-media-controls-buttons-styles.html:54
> +const buttonPositions = new Map([

I think this could just be an array.

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 291633 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291633&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:47
> > +        this._leftContainer = new ButtonsContainer({
> > +            buttons: [this.playPauseButton, this.skipBackButton],
> > +            cssClassName: "left",
> > +            padding: 24,
> > +            margin: 24
> > +        });
> > +
> > +        this._rightContainer = new ButtonsContainer({
> > +            buttons: [this.muteButton, this.airplayButton, this.pipButton, this.tracksButton, this.fullscreenButton],
> > +            cssClassName: "right",
> > +            padding: 24,
> > +            margin: 24
> > +        });
> 
> Should we be calling these left and right? What about RTL mode?

RTL for media controls only affects the direction of the volume control in fullscreen. I think it's fine to keep these terms.

> > Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:101
> > +        // Reset dropped buttons.
> > +        this._rightContainer.buttons.concat(this._leftContainer.buttons).forEach(button => delete button.dropped);
> > +
> > +        this._leftContainer.layout();
> > +        this._rightContainer.layout();
> > +
> > +        this.timeControl.width = this.width - this._leftContainer.width - this._rightContainer.width;
> > +
> > +        if (this.timeControl.isSufficientlyWide) {
> > +            this.controlsBar.insertBefore(this.timeControl, this._rightContainer);
> > +            this.timeControl.x = this._leftContainer.width;
> > +        } else {
> > +            this.timeControl.remove();
> > +            // Since we don't have enough space to display the scrubber, we may also not have
> > +            // enough space to display all buttons in the left and right containers, so gradually drop them.
> > +            for (let control of [this.airplayButton, this.pipButton, this.tracksButton, this.muteButton, this.skipBackButton, this.fullscreenButton]) {
> > +                // Nothing left to do if the combined container widths is shorter than the available width.
> > +                if (this._leftContainer.width + this._rightContainer.width < this.width)
> > +                    break;
> > +
> > +                // If the control was already not participating in layout, we can skip it.
> > +                if (!control.visible)
> > +                    continue;
> > +
> > +                // This control must now be dropped.
> > +                control.dropped = true;
> > +
> > +                this._leftContainer.layout();
> > +                this._rightContainer.layout();
> > +            }
> > +        }
> 
> I don't really understand how this works. We start by deleting all buttons
> that are dropped.

We delete the `dropped` property on all buttons, so that their dropped status may be reconsidered for this new layout. The `dropped` property is an informal protocol of the ButtonsContainer class to distinguish buttons that may not be shown for a given layout width.

> But I don't understand the "delete button.dropped" line (72). Isn't
> button.dropped just a boolean?

It is, but it's not a property of Button, so we just remove it since it's only added for the purpose of layout.

> Did you mean to do .filter(button => button.dropped).forEach(button =>
> delete button)

No, see above.

> Then we go on to the loop over the left-side controls. If we need to drop
> the control, we set dropped to true, and call layout again. Why do you need
> to layout the left side after this? And won't it trigger this layout
> function to run again, since the children may have changed sizes? If so,
> aren't you doing more work than necessary?

Buttons that are candidates to be dropped happen to be contained in both the left and right containers. So, for each button, in the order that they may be dropped, we update the layout of both the left and right containers, to ensure that their `width` property is updated, and then recheck whether the combined width of the two button containers now fit in the width of the controls bar. Remember that calling layout() on the buttons container is only touching the LayoutNode tree and not the DOM, so it's a cheap operation, which lets us keep things simple.

One thing we could do is only relayout the buttons container in which the button we're processing is contained, but that would require a lookup on both containers to see where the button belongs.

> > Source/WebCore/Modules/modern-media-controls/controls/macos-inline-media-controls.js:104
> > +        this._volumeSliderContainer.x = this._rightContainer.x + this.muteButton.x;
> 
> What if muteButton has been dropped?

Then the volume slider position will be irrelevant because the volume slider will never appear since it's tied to the muteButton being hovered.

> > LayoutTests/media/modern-media-controls/macos-inline-media-controls/macos-inline-media-controls-buttons-styles.html:54
> > +const buttonPositions = new Map([
> 
> I think this could just be an array.

You're right, will change that.

[Antoine Quint]

Created attachment 291711 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291711 [details]
Patch for landing

Clearing flags on attachment: 291711

Committed r207373: <http://trac.webkit.org/changeset/207373>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.