Skip to content

Commit

Permalink
Bug 1861869 - Don't re-create the UA widget when entering PiP. r=mcon…
Browse files Browse the repository at this point in the history
…ley,pip-reviewers,reusable-components-reviewers

This is the actual fix. The issue is that, after the regressing bug, the
video element will only dispatch resized* events when:

 * The frame is recreated.
 * The video actually resizes.

And when going into PiP neither is happening, so we create a new impl,
but never update the "reflowed" dimensions.

Long term we should probably rewrite this to use ResizeObserver rather
than special code, but this should be a good perf improvement, and fix
the correctness issue, so seems worth doing.

Depends on D203496

Differential Revision: https://phabricator.services.mozilla.com/D203497
  • Loading branch information
emilio committed Mar 6, 2024
1 parent 7287074 commit 0733a21
Showing 1 changed file with 15 additions and 20 deletions.
35 changes: 15 additions & 20 deletions toolkit/content/widgets/videocontrols.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,8 @@ this.VideoControlsWidget = class {
// the underlying element state hasn't changed in ways that we
// care about. This can happen if the property is set again
// without a value change.
if (
this.impl &&
this.impl.constructor == newImpl &&
this.impl.elementStateMatches(this.element)
) {
if (this.impl && this.impl.constructor == newImpl) {
this.impl.onchange();
return;
}
if (this.impl) {
Expand Down Expand Up @@ -458,10 +455,10 @@ this.VideoControlsImplWidget = class {
this.statusIcon.setAttribute("type", "error");
this.updateErrorText();
this.setupStatusFader(true);
} else if (VideoControlsWidget.isPictureInPictureVideo(this.video)) {
this.setShowPictureInPictureMessage(true);
}

this.updatePictureInPictureMessage();

if (this.video.readyState >= this.video.HAVE_METADATA) {
// According to the spec[1], at the HAVE_METADATA (or later) state, we know
// the video duration and dimensions, which means we can calculate whether or
Expand Down Expand Up @@ -934,6 +931,8 @@ this.VideoControlsImplWidget = class {
// Since this event come from the layout, this is the only place
// we are sure of that probing into layout won't trigger or force
// reflow.
// FIXME(emilio): We should rewrite this to just use
// ResizeObserver, probably.
this.reflowTriggeringCallValidator.isReflowTriggeringPropsAllowed = true;
this.updateReflowedDimensions();
this.reflowTriggeringCallValidator.isReflowTriggeringPropsAllowed = false;
Expand Down Expand Up @@ -1095,7 +1094,10 @@ this.VideoControlsImplWidget = class {
);
},

setShowPictureInPictureMessage(showMessage) {
updatePictureInPictureMessage() {
let showMessage =
!this.hasError() &&
VideoControlsWidget.isPictureInPictureVideo(this.video);
this.pictureInPictureOverlay.hidden = !showMessage;
this.isShowingPictureInPictureMessage = showMessage;
},
Expand Down Expand Up @@ -2899,9 +2901,8 @@ this.VideoControlsImplWidget = class {
this.l10n.translateRoots();
}

elementStateMatches(element) {
let elementInPiP = VideoControlsWidget.isPictureInPictureVideo(element);
return this.isShowingPictureInPictureMessage == elementInPiP;
onchange() {
this.Utils.updatePictureInPictureMessage();
}

teardown() {
Expand Down Expand Up @@ -3061,9 +3062,7 @@ this.NoControlsMobileImplWidget = class {
this.Utils.init(this.shadowRoot);
}

elementStateMatches() {
return true;
}
onchange() {}

teardown() {
this.Utils.terminate();
Expand Down Expand Up @@ -3111,9 +3110,7 @@ this.NoControlsPictureInPictureImplWidget = class {
this.shadowRoot.firstElementChild.setAttribute("localedir", direction);
}

elementStateMatches() {
return true;
}
onchange() {}

teardown() {}

Expand Down Expand Up @@ -3288,9 +3285,7 @@ this.NoControlsDesktopImplWidget = class {
this.Utils.init(this.shadowRoot, this.prefs);
}

elementStateMatches() {
return true;
}
onchange() {}

teardown() {
this.Utils.terminate();
Expand Down

0 comments on commit 0733a21

Please sign in to comment.