Bug 236729 – [iOS][Modern Media Controls] Add an alternative overflow icon

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

[Peng Liu]

[Modern Media Controls] Add an alternative overflow icon

[Peng Liu]

Created attachment 452231 [details]
Patch

[Peng Liu]

Created attachment 452233 [details]
Patch

[Devin Rousso]

Comment on attachment 452233 [details]
Patch

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

r=me

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:38
> +    Ellipsis        : { name: "Ellipsis", type: "svg", label: UIString("More\u2026") },

please alphabetize this by moving it right above `EllipsisCircle`

> Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:94
> +    overflowButtonHasCircle() {

Style: `{` goes on a separate line :)

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:46
> +        if (layoutTraits.overflowButtonHasCircle())

I think you can actually get to the `layoutTraits` inside `OverflowButton` via something like `layoutDelegate?.layoutTraits`.  So I'd leave this as-is and then change the `constructor` of `OverflowButton` to be like so:
```
super({
    cssClassName: "overflow",
    iconName: layoutDelegate?.layoutTraits.overflowButtonHasCircle() ? Icons.EllipsisCircle : Icons.Ellipsis,
    layoutDelegate,
});
```

[Peng Liu]

Comment on attachment 452233 [details]
Patch

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

>> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:38
>> +    Ellipsis        : { name: "Ellipsis", type: "svg", label: UIString("More\u2026") },
> 
> please alphabetize this by moving it right above `EllipsisCircle`

Sure!

>> Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js:94
>> +    overflowButtonHasCircle() {
> 
> Style: `{` goes on a separate line :)

Yes.

>> Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:46
>> +        if (layoutTraits.overflowButtonHasCircle())
> 
> I think you can actually get to the `layoutTraits` inside `OverflowButton` via something like `layoutDelegate?.layoutTraits`.  So I'd leave this as-is and then change the `constructor` of `OverflowButton` to be like so:
> ```
> super({
>     cssClassName: "overflow",
>     iconName: layoutDelegate?.layoutTraits.overflowButtonHasCircle() ? Icons.EllipsisCircle : Icons.Ellipsis,
>     layoutDelegate,
> });
> ```

I like this idea! Thanks!

[Peng Liu]

Created attachment 452288 [details]
Patch for landing

[Aakash Jain]

I cancelled https://ews-build.webkit.org/#/builders/68/builds/8368 to speed up ios-wk2 queue (as it is backlogged). That build indicated 30+ layout-test failures on Patch 452233. Please check.

[Peng Liu]

Created attachment 452392 [details]
Fix test failures

[Peng Liu]

Created attachment 452393 [details]
Fix an issue on watchOS

[Peng Liu]

Created attachment 452448 [details]
[fast-cq] Revise the patch based on Devin's suggestions

[EWS]

Committed r290133 (247477@main): <https://commits.webkit.org/247477@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452448 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89150124>