Bug 230692 - [Media Controls] Allow for a single mute and volume button
Summary: [Media Controls] Allow for a single mute and volume button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-23 08:55 PDT by Antoine Quint
Modified: 2021-09-23 12:08 PDT (History)
11 users (show)

See Also:

Attachments
Patch (30.22 KB, patch)
2021-09-23 09:01 PDT, Antoine Quint 		dino: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-09-23 08:55:24 PDT 
[Media Controls] Allow for a single mute and volume button
Comment 1 Antoine Quint 2021-09-23 09:01:47 PDT 
Created attachment 439049 [details]
Patch
Comment 2 Antoine Quint 2021-09-23 09:01:53 PDT 
<rdar://problem/79956248>
Comment 3 Antoine Quint 2021-09-23 10:44:58 PDT 
Committed r282971 (242061@main): <https://commits.webkit.org/242061@main>
Comment 4 Devin Rousso 2021-09-23 12:08:18 PDT 
Comment on attachment 439049 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/range-button.js:75
> +        if (event.currentTarget === this.element && event.type == "pointerdown")

oops `==`

> Source/WebCore/Modules/modern-media-controls/controls/range-button.js:101
> +            if (this.uiDelegate && typeof this.uiDelegate.controlValueWillStartChanging === "function")
> +                this.uiDelegate.controlValueWillStartChanging(this);

NIT: you can simplify this as
```
    this.uiDelegate?.controlValueWillStartChanging?.(this);
```

> Source/WebCore/Modules/modern-media-controls/controls/range-button.js:134
> +            if (this.uiDelegate && typeof this.uiDelegate.controlValueDidStopChanging === "function")
> +                this.uiDelegate.controlValueDidStopChanging(this);

ditto (:100)
```
    this.uiDelegate?.controlValueDidStopChanging?.(this);
```

> Source/WebCore/Modules/modern-media-controls/controls/range-button.js:138
> +            if (this._enabled && this.uiDelegate && typeof this.uiDelegate.buttonWasPressed === "function")
> +                this.uiDelegate.buttonWasPressed(this);

ditto (:100)
```
    if (this._enabled)
        this.uiDelegate?.buttonWasPressed?.(this);
```

> Source/WebCore/Modules/modern-media-controls/controls/volume-button.js:26
> +class VolumeButton extends RangeButton

I wonder if instead of basically duplicating the entirety of `MuteButton` we could've had `MuteButton` extend from `RangeButton` and just have a `set allowsSlidingToAdjustVolume` to switch behavior at runtime or something 🤔