Bug 221594 – [iOS] Add support for the language/subtitle tracks button using `UIMenu`

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

[Devin Rousso]

instead of `TracksPanel` like on macOS (which would be possible but difficult to match the exact behavior/style of UIKit) just use a native `UIContextMenuInteraction` and `UIMenu`

[Radar WebKit Bug Importer]

<rdar://problem/74129129>

[Devin Rousso]

Created attachment 419678 [details]
Patch

[Devin Rousso]

Created attachment 419680 [details]
Patch

[Eric Carlson]

Comment on attachment 419680 [details]
Patch

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

The WebCore changes look good to me modulo my minor comments, but someone that knows more about WKActionSheetAssistant than I do should review those changes.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:327
> +
> +    HashMap<MediaControlsContextMenuItem::ID, Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>> idMap;

Nit: something like `using MenuTrackItem = Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>` would make this easier to parse.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:407
> +        auto selectedItem = idMap.get(selectedItemID);

Might be worth adding something like `ASSERT(selectedItem != HashTraits<MediaControlsContextMenuItem::ID>::emptyValue())`

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:419
> +                for (auto& track : idMap.values()) {
> +                    if (auto* textTrack = WTF::get_if<RefPtr<TextTrack>>(track))
> +                        (*textTrack)->setMode(TextTrack::Mode::Disabled);
> +                }

Doesn't HTMLMediaElement::setSelectedTextTrack already take care of this?

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:68
> +    [Conditional=MEDIA_CONTROLS_CONTEXT_MENUS, EnabledBySetting=MediaControlsContextMenus] undefined showMediaControlsContextMenu(HTMLElement target, ContextMenuOptions options);

Does this need `EnabledAtRuntime=ModernMediaControls`, or is that obsolete (and so should be removed everywhere)?

> Source/WebCore/page/MediaControlsContextMenuItem.h:48
> +struct MediaControlsContextMenuItem {
> +    using ID = uint64_t;
> +    static constexpr ID invalidID = 0;
> +
> +    ID id { invalidID };
> +    String title;
> +    String icon;
> +    bool isChecked { false };
> +    Vector<MediaControlsContextMenuItem> children;
> +
> +    template<class Encoder> void encode(Encoder&) const;
> +    template<class Decoder> static Optional<MediaControlsContextMenuItem> decode(Decoder&);
> +};

Isn't `children` always empty except for the last entry? It should either be Optional<> or you could define one struct for the menu and another for the items?

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1077
> +- (NSArray *)currentAvailableMediaControlsContextMenuItems

s/currentAvailableMediaControlsContextMenuItems/currentlyAvailableMediaControlsContextMenuItems/

[Devin Rousso]

Comment on attachment 419680 [details]
Patch

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

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:327
>> +    HashMap<MediaControlsContextMenuItem::ID, Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>> idMap;
> 
> Nit: something like `using MenuTrackItem = Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>` would make this easier to parse.

ooo good idea =D

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:407
>> +        auto selectedItem = idMap.get(selectedItemID);
> 
> Might be worth adding something like `ASSERT(selectedItem != HashTraits<MediaControlsContextMenuItem::ID>::emptyValue())`

I think `HashTable::checkKey` already does this

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:419
>> +                }
> 
> Doesn't HTMLMediaElement::setSelectedTextTrack already take care of this?

I was emulating the code inside `TrackSupport.prototype.tracksPanelSelectionDidChange`.  AFAICT `HTMLMediaElement::setSelectedTextTrack` doesn't do this if the given `trackToSelect` is the special "automatic" or "off" track.

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:68
>> +    [Conditional=MEDIA_CONTROLS_CONTEXT_MENUS, EnabledBySetting=MediaControlsContextMenus] undefined showMediaControlsContextMenu(HTMLElement target, ContextMenuOptions options);
> 
> Does this need `EnabledAtRuntime=ModernMediaControls`, or is that obsolete (and so should be removed everywhere)?

Oh yeah good call.  I'll add that as well :)

>> Source/WebCore/page/MediaControlsContextMenuItem.h:48
>> +};
> 
> Isn't `children` always empty except for the last entry? It should either be Optional<> or you could define one struct for the menu and another for the items?

The idea here is to be able to build a tree.  If a video has both audio and text tracks, this would be used to create two submenus that each have children.  A couple of the LayoutTests I wrote dump this structure so you can see it.

IMO `Optional<Vector<...>>` is kinda redundant in this case because if a menu item has children it's not selectable (at the very least I think this is the case for macOS/iOS menus).  Yes I could add distinct subclasses for menu vs action, but I think `children.isEmpty()` is indicator enough as to whether or not it's a menu or an action and distinct subclasses would just make this code more complicated.

[Wenson Hsieh]

Comment on attachment 419680 [details]
Patch

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

>>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:68
>>> +    [Conditional=MEDIA_CONTROLS_CONTEXT_MENUS, EnabledBySetting=MediaControlsContextMenus] undefined showMediaControlsContextMenu(HTMLElement target, ContextMenuOptions options);
>> 
>> Does this need `EnabledAtRuntime=ModernMediaControls`, or is that obsolete (and so should be removed everywhere)?
> 
> Oh yeah good call.  I'll add that as well :)

IIRC, `EnabledAtRuntime` is for runtime-enabled features (`WebCore::RuntimeEnabledFeatures`), and allows the bindings generator guards the IDL with a runtime enabled feature check:

```
    if ($context->extendedAttributes->{EnabledAtRuntime}) {
        assert("Must specify value for EnabledAtRuntime.") if $context->extendedAttributes->{EnabledAtRuntime} eq "VALUE_IS_MISSING";

        AddToImplIncludes("RuntimeEnabledFeatures.h");

        my @flags = split(/&/, $context->extendedAttributes->{EnabledAtRuntime});
        foreach my $flag (@flags) {
            push(@conjuncts, "RuntimeEnabledFeatures::sharedFeatures()." . ToMethodName($flag) . "Enabled()");
        }
    }
```

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1041
> +    NSMutableArray *array = [NSMutableArray array];

Nit - `-arrayWithCapacity:`?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7170
> +        completionHandler(selectedItemID);

Nit - does it work if you just pass in `WTFMove(completionHandler)` here, or is the outer completion handler needed?

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:1040
> +    if (UIView *calloutBar = UICalloutBar.activeCalloutBar) {

Nit - I need to double check, but I suspect `UICalloutBar.activeCalloutBar` is non-nil even if the callout bar is not currently shown. (The point of `activeCalloutBar`, `+[UIKeyboardImpl activeInstance]`, and the like are to avoid the cost of instantiating these singleton objects too early, usually during app launch).

[Devin Rousso]

Comment on attachment 419680 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1041
>> +    NSMutableArray *array = [NSMutableArray array];
> 
> Nit - `-arrayWithCapacity:`?

This was just copypasta from above, but I'm happy to change it :)

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7170
>> +        completionHandler(selectedItemID);
> 
> Nit - does it work if you just pass in `WTFMove(completionHandler)` here, or is the outer completion handler needed?

:extreme facepalm:

>> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:1040
>> +    if (UIView *calloutBar = UICalloutBar.activeCalloutBar) {
> 
> Nit - I need to double check, but I suspect `UICalloutBar.activeCalloutBar` is non-nil even if the callout bar is not currently shown. (The point of `activeCalloutBar`, `+[UIKeyboardImpl activeInstance]`, and the like are to avoid the cost of instantiating these singleton objects too early, usually during app launch).

Oh wow I completely missed that it was checking for `calloutBar.window`.  Will change.  Great catch!

[Devin Rousso]

Created attachment 419751 [details]
Patch

[Wenson Hsieh]

Comment on attachment 419751 [details]
Patch

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

The test harness and context menu changes look good to me. I'm not familiar with the media and media-controls side of things though, so perhaps one of the media folks could take a closer look!

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:685
>  - (void)ensureContextMenuInteraction

Nit - should this become `ensureContextMenuInteractions`? (Or perhaps split this into two ensure methods, since there's no need to create and add `_dataDetectorContextMenuInteraction` when showing the media context menu).

If you do decide to split this into two ensures, you could also just return the interaction from them, and then do `[self.ensureMediaControlsContextMenuInteraction _presentMenuAtLocation:_mediaControlsContextMenuTargetFrame.center()];` below.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1071
> +            result[@"checked"] = [NSNumber numberWithBool:YES];

Nit - this can just be @YES.

[Devin Rousso]

Comment on attachment 419751 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:685
>>  - (void)ensureContextMenuInteraction
> 
> Nit - should this become `ensureContextMenuInteractions`? (Or perhaps split this into two ensure methods, since there's no need to create and add `_dataDetectorContextMenuInteraction` when showing the media context menu).
> 
> If you do decide to split this into two ensures, you could also just return the interaction from them, and then do `[self.ensureMediaControlsContextMenuInteraction _presentMenuAtLocation:_mediaControlsContextMenuTargetFrame.center()];` below.

oh yeah good idea

[Devin Rousso]

Created attachment 419777 [details]
Patch

also split `removeContextMenuInteraction` and move it outside of the lambda so that if a second request to show the contextmenu comes in while the first one is dismissing then it's not removed

[EWS]

Committed r272630: <https://commits.webkit.org/r272630>

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

[Devin Rousso]

unreviewed build fix r272631: <https://commits.webkit.org/r272631>