rdar://83173597
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].