Bug 232899 – Refactor some image overlay logic to work with built-in media controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

rdar://83173597

[Wenson Hsieh]

Created attachment 443741 [details]
For EWS

[Wenson Hsieh]

Created attachment 443750 [details]
Remove unnecessary stylesheet change

[Wenson Hsieh]

Created attachment 443761 [details]
Patch

[Antoine Quint]

Comment on attachment 443761 [details]
Patch

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

r=me for the media controls parts.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:350
> +    static MainThreadNeverDestroyed<const AtomString> className("media-controls-container", AtomString::ConstructFromLiteral);

Maybe we could expose this through the IDL for the media controls to use?

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:94
> +    -webkit-user-select: none;

Should we add the "We always want to prevent the iOS magnifier UI to be shown." comment removed from `:host`?

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:129
> -}
>  \ No newline at end of file
> +}

Nice.

[Wenson Hsieh]

Comment on attachment 443761 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:350
>> +    static MainThreadNeverDestroyed<const AtomString> className("media-controls-container", AtomString::ConstructFromLiteral);
> 
> Maybe we could expose this through the IDL for the media controls to use?

Good call — looking into that now!

>> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:94
>> +    -webkit-user-select: none;
> 
> Should we add the "We always want to prevent the iOS magnifier UI to be shown." comment removed from `:host`?

Makes sense! Will add a comment to that effect here.

>> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:129
>> +}
> 
> Nice.

Xcode is...oddly insistent on this change :(

[Devin Rousso]

Comment on attachment 443761 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/controls-bar.css:31
> +    z-index: 1;

IIRC the only other thing that uses `z-index` is `video::-webkit-media-text-track-container`.  I don't think there's ever any overlap (at least there shouldn't be), but it might be worth testing this on some `<video>` that have subtitles just to be sure :)

[Wenson Hsieh]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 443761 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443761&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/controls/controls-bar.css:31
> > +    z-index: 1;
> 
> IIRC the only other thing that uses `z-index` is
> `video::-webkit-media-text-track-container`.  I don't think there's ever any
> overlap (at least there shouldn't be), but it might be worth testing this on
> some `<video>` that have subtitles just to be sure :)

That's a good point!

From my testing with captions in HTML video (alongside built-in controls), the text track container dodges the bottom controls bar anyways, so I *think* we should be okay here.

[Wenson Hsieh]

Created attachment 443843 [details]
Adjust for review comments

[Wenson Hsieh]

Created attachment 443857 [details]
Patch for landing

[EWS]

Committed r285609 (244113@main): <https://commits.webkit.org/244113@main>

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