Removing the controls attribute from a <video> element does not tear down the controls shadow DOM nor cancel event listeners.
<rdar://problem/33793004>
Created attachment 333553 [details] Patch
Created attachment 333554 [details] Patch
Comment on attachment 333554 [details] Patch Attachment 333554 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6443628 New failing tests: http/tests/contentextensions/text-track-blocked.html
Created attachment 333555 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
So, we already do the right thing when entering or exiting fullscreen on iOS: _updateiOSFullscreenProperties() { // On iOS, we want to make sure not to update controls when we're in fullscreen since the UI // will be completely invisible. if (!(this.layoutTraits & LayoutTraits.iOS)) return; const isFullscreen = this.isFullscreen; if (isFullscreen) this._supportingObjects.forEach(supportingObject => supportingObject.disable()); else this._supportingObjects.forEach(supportingObject => supportingObject.enable()); this.controls.visible = !isFullscreen; } Sounds to me like we could extend this to dealing with changes to the controls attribute as well. Setting .visible on the controls will not remove the controls element itself but will prevent any content from being added to its subtree and so the shadow root will be mostly empty.
> Sounds to me like we could extend this to dealing with changes to the > controls attribute as well. Setting .visible on the controls will not remove > the controls element itself but will prevent any content from being added to > its subtree and so the shadow root will be mostly empty. It’s a bigger issue than that; we set up all the controls and listeners and repaint timers even when there’s just a visible text track, and the media element never had the controls attribute in the first place. Though we should definitely disable all the listeners when the controls are invisible, but that is a separate bug.
Created attachment 333599 [details] Patch
There were some issues with Jer's patch, specifically LTR state wouldn't be preserved when turning controls back on and the AirPlay and PiP placards wouldn't be displayed. I have a patch coming up that addresses the reported issues and preserves LTR and placards.
Created attachment 333745 [details] Patch
Comment on attachment 333745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333745&action=review Do we have a test for verifying that the AirPlay plackard is shown when initiating remote playback from a media element without a controls attribute? > Source/WebCore/Modules/modern-media-controls/media/placard-support.js:51 > + if (this.mediaController.isFullscreen) Does this need an iOS check? Presumably we still want the placards in Mac full screen?
(In reply to Jer Noble from comment #11) > Comment on attachment 333745 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333745&action=review > > Do we have a test for verifying that the AirPlay plackard is shown when > initiating remote playback from a media element without a controls attribute? > > > Source/WebCore/Modules/modern-media-controls/media/placard-support.js:51 > > + if (this.mediaController.isFullscreen) > > Does this need an iOS check? Presumably we still want the placards in Mac > full screen? This code wouldn't be called on macOS fullscreen since controls will be forced in that scenario.
Committed r228445: <https://trac.webkit.org/changeset/228445>
This caused a regression on iOS where controls were no longer disabled while in fullscreen, see https://bugs.webkit.org/show_bug.cgi?id=182830. EWS didn't catch the failure but internal bots did since the test for this behaviour only runs internally due to usage of touch events.
This caused https://bugs.webkit.org/show_bug.cgi?id=183074.