Bug 232899

Summary: Refactor some image overlay logic to work with built-in media controls
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, hi, jer.noble, joepeck, kondapallykalyan, philipj, sergio, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For EWS
none
Remove unnecessary stylesheet change
none
Patch
none
Adjust for review comments
thorton: review+
Patch for landing none

Description Wenson Hsieh 2021-11-09 12:13:27 PST
rdar://83173597
Comment 1 Wenson Hsieh 2021-11-09 15:58:47 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-11-09 16:20:37 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-11-09 17:32:12 PST
Created attachment 443761 [details]
Patch
Comment 4 Antoine Quint 2021-11-10 08:46:42 PST
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.
Comment 5 Wenson Hsieh 2021-11-10 09:09:58 PST
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 :(
Comment 6 Devin Rousso 2021-11-10 11:25:44 PST
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 :)
Comment 7 Wenson Hsieh 2021-11-10 11:35:33 PST
(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.
Comment 8 Wenson Hsieh 2021-11-10 12:09:26 PST
Created attachment 443843 [details]
Adjust for review comments
Comment 9 Wenson Hsieh 2021-11-10 13:24:32 PST
Created attachment 443857 [details]
Patch for landing
Comment 10 EWS 2021-11-10 16:03:15 PST
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].