Summary: | Refactor some image overlay logic to work with built-in media controls | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Component: | Platform | Assignee: | 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
Wenson Hsieh
2021-11-09 12:13:27 PST
Created attachment 443741 [details]
For EWS
Created attachment 443750 [details]
Remove unnecessary stylesheet change
Created attachment 443761 [details]
Patch
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 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 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 :) (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. Created attachment 443843 [details]
Adjust for review comments
Created attachment 443857 [details]
Patch for landing
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]. |