Removing the controls attribute from a <video> element does not tear down the controls shadow DOM nor cancel event listeners.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Removing the controls attribute from a <video> element does not tear down the controls shadow DOM nor cancel event listeners.

[Jer Noble]

<rdar://problem/33793004>

[Jer Noble]

Created attachment 333553 [details]
Patch

[Jer Noble]

Created attachment 333554 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

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.

[Jer Noble]

> 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.

[Jer Noble]

Created attachment 333599 [details]
Patch

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 333745 [details]
Patch

[Jer Noble]

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?

[Antoine Quint]

(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.

[Antoine Quint]

Committed r228445: <https://trac.webkit.org/changeset/228445>

[Antoine Quint]

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.

[Antoine Quint]

This caused https://bugs.webkit.org/show_bug.cgi?id=183074.