WebVTT subtitle does not reposition after control bar fades

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

This is a regression caused by Part VIII of bug 1444489.

The code here checks controlBar.clientHeight, and it will be non-zero since we no longer set `display: none` to the controll bar after the transition.

https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/dom/media/webvtt/vtt.jsm#1017
Comment hidden (mozreview-request)
Comment hidden (obsolete)
Comment hidden (mozreview-request)
Comment hidden (obsolete)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8963133 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
Comment on attachment 8963133 [details]
Bug 1449532 - Part I, Backed out changeset 99fc41ec7ce9 (Bug 1444489 Part VIII)

https://reviewboard.mozilla.org/r/232000/#review237938

I'm assuming this is a straight backout, so rs=me for that.
Attachment #8963133 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8963134 [details]
Bug 1449532 - Part II, Use Web Animation API to animate video control transition

https://reviewboard.mozilla.org/r/232002/#review237902

::: toolkit/content/widgets/videocontrols.xml:1030
(Diff revision 5)
> +          animation = new Animation(new KeyframeEffect(
> +            element, animationProp.keyframes, animationProp.options));

I'm confused, the MDN docs say this is behind a pref, dom.animations-api.core.enabled, which is false by default (the pref, at least exists and is indeed false for me by default on beta). There are no comments on the bug about any of this, so I don't understand if this is supposed to be possible because we're in a XBL scope, despite the pref being false, or something else... Given the lack of comments, I'm assuming that you used Nightly and didn't realize this is preffed off on release, so we can't ship this; it'll break on beta/release.

::: toolkit/themes/shared/media/videocontrols.css:68
(Diff revision 5)
>       see videocontrols.xml and search for |-width|. */
>    --scrubberStack-width: 84px;
>    --volumeStack-width: 64px;
>  }
>  
> -.controlsContainer [hidden="true"],
> +.controlsContainer [hidden],

Do we still need this? I'm confused how we're using the 'hidden' property and yet need to do work to make the attribute do its job...
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs)
Brian, do you know what the status is on the above API?
Flags: needinfo?(bbirtles)
We had the conversation on Twitter a few hours ago. He has suggested an alternative to the constructor & said the finished promise is ready to ship. Regardless, I've got a patch ready to polyfill the finished promise.
Flags: needinfo?(bbirtles)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8963134 [details]
Bug 1449532 - Part II, Use Web Animation API to animate video control transition

https://reviewboard.mozilla.org/r/232002/#review237902

> I'm confused, the MDN docs say this is behind a pref, dom.animations-api.core.enabled, which is false by default (the pref, at least exists and is indeed false for me by default on beta). There are no comments on the bug about any of this, so I don't understand if this is supposed to be possible because we're in a XBL scope, despite the pref being false, or something else... Given the lack of comments, I'm assuming that you used Nightly and didn't realize this is preffed off on release, so we can't ship this; it'll break on beta/release.

Yeah I overlooked. Sorry about that. I've add the needed polyfill and replacement to the feature on the new patch.

> Do we still need this? I'm confused how we're using the 'hidden' property and yet need to do work to make the attribute do its job...

Appearently we still do. I have not look into why.
The 'finished' promise and the KeyframeEffect constructor are only enabled when the dom.animations-api.core.enabled is on (Nightly-only) and for Chrome callers.

I'll try to get things in a state where we can ship 'finished' in 61 but it's currently blocked on web-platform-tests syncing and the fact that my team no longer works on this.
Comment hidden (mozreview-request)

Comment 20

a year ago
I'm not familiar enough with the APIs and bugs (like that opacity CSS thing?) here to do this review quickly, and today + Monday are public holidays in the UK, so the earliest I'll be able to review this is Tuesday. If that's not quick enough, you might want to switch to a US-based reviewer like :jaws . Sorry!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs)
Attachment #8964091 - Flags: review?(gijskruitbosch+bugs)

Comment 23

a year ago
mozreview-review
Comment on attachment 8963134 [details]
Bug 1449532 - Part II, Use Web Animation API to animate video control transition

https://reviewboard.mozilla.org/r/232002/#review238756

Some cosmetic/organizational optimizations below, but on the whole this LGTM, thanks! The only real qualm I have is with what happens to aborted animations, because the MDN docs don't say...

::: toolkit/content/widgets/videocontrols.xml:298
(Diff revision 9)
> -                  control.setAttribute("hidden", "true");
> +                if (control._isHiddenExplicitly || control._isHiddenByAdjustment) {
> +                  control.setAttribute("hidden", "");
>                  } else {
>                    control.removeAttribute("hidden");
>                  }

Could you factor this out into a utility method that you just call from both `.hidden` and `.hideByAdjustment`, to avoid the duplication?

::: toolkit/content/widgets/videocontrols.xml:1026
(Diff revision 9)
> +      startFade(element, fadeIn, immediate = false) {
> +        if (element == this.controlBar && fadeIn) {
>            // Bug 493523, the scrubber doesn't call valueChanged while hidden,
>            // so our dependent state (eg, timestamp in the thumb) will be stale.
>            // As a workaround, update it manually when it first becomes unhidden.
>            if (element.hidden) {

Nit: can you move this `.hidden` check into the condition while we're changing this anyway?

::: toolkit/content/widgets/videocontrols.xml:1071
(Diff revision 9)
> +            if (element == this.controlBar) {
> +              this.onControlBarTransitioned();
> +            }

What happens if the binding gets rebound mid-animation because of fullscreen etc. ? Does this still fire? If so, is `this.controlBar` overwritten with the new one at that point (meaning we don't call `onControlbarTransitioned`), or no? If we don't call it, does that matter? (I tried to figure out if the finished promise resolves or rejects when the element being animated is destroyed mid-animation, but reading https://developer.mozilla.org/en-US/docs/Web/API/Animation/finished did not enlighten me.)

Do we need to catch rejections of the finished promise (ie can that happen) ?

Separately, nit: can we rename to `onControlBarAnimationFinished` or similar? :-)

::: toolkit/content/widgets/videocontrols.xml:1085
(Diff revision 9)
> +          element.classList.add("fadeout");
> +          element.classList.remove("fadein");
> +          let finishedPromise;
> +          if (!immediate) {
> +            animation.playbackRate = -1;
> +            animation.play();
> +            finishedPromise = animation.finished;
> +          } else {
> +            animation.cancel();
> +            finishedPromise = Promise.resolve();
> +          }
> +          finishedPromise.then(() => {
> +            if (element == this.controlBar) {
> +              this.onControlBarTransitioned();
> +            }
> +            element.classList.remove("fadeout");
> -        element.hidden = true;
> +            element.hidden = true;
> +          });

This is basically the same between the two branches of the `if (fadeIn)` conditional.

Can we keep the early returns in the branches, as well as the `element.hidden = false` bit in the fadeIn case and the cursor hiding in the fadeout case, and then move the rest out, doing something like:

```js
element.classList.toggle("fadeout", !fadeIn);
element.classList.toggle("fadein", fadeIn);
let finishedPromise;
if (!immediate) {
  animation.playbackRate = fadeIn ? 1 : -1;
  animation.play();
  finishedPromise = animation.finished;
} else {
  animation.cancel();
  finishedPromise = Promise.resolve();
}
finishedPromise.then(() => {
  if (element == this.controlBar) {
    this.onControlBarTransitioned();
  }
  element.classList.remove(fadeIn ? "fadein" : "fadeout");
  if (!fadeIn) {
    element.hidden = true;
  }
});
```
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8964091 [details]
Bug 1449532 - Part III, Polyfill Web Animation API features

https://reviewboard.mozilla.org/r/232886/#review238790

r=me but please can we use the polyfill even when we do support the original thing, to avoid surprises related to scheduling issues once the code hits a build where the pref is turned off? We can remove it when we ship the builtin support. Thanks!

::: toolkit/content/widgets/videocontrols.xml:1040
(Diff revision 1)
> +          then(fn) {
> +            this.fn = fn;
> +          }

This calls the handler synchronously from the event handler. That's not how promise resolution .then() callbacks normally work (ie the scheduling is different). I don't think it really matters for this code, especially as you're already having to hack around the other bug here, but it may be worth a comment, as we'll need to check this more carefully when we do ship the full implementation, and it has the potential of hiding bugs on nightly until code hits beta/devedition... so that's a bit unfortunate.

Perhaps it would be worth using the polyfill even on builds where `animation.finished` support is present? That would avoid nasty surprises when merging beta. I realize it's not ideal, but equally the overhead of the polyfill in this case is very minimal so it doesn't seem too big a deal, so probably worth it to avoid the risk of beta/release-only bugs?
Attachment #8964091 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8963134 [details]
Bug 1449532 - Part II, Use Web Animation API to animate video control transition

https://reviewboard.mozilla.org/r/232002/#review238756

> What happens if the binding gets rebound mid-animation because of fullscreen etc. ? Does this still fire? If so, is `this.controlBar` overwritten with the new one at that point (meaning we don't call `onControlbarTransitioned`), or no? If we don't call it, does that matter? (I tried to figure out if the finished promise resolves or rejects when the element being animated is destroyed mid-animation, but reading https://developer.mozilla.org/en-US/docs/Web/API/Animation/finished did not enlighten me.)
> 
> Do we need to catch rejections of the finished promise (ie can that happen) ?
> 
> Separately, nit: can we rename to `onControlBarAnimationFinished` or similar? :-)

When the animation is canceled, the promise will be rejected. We only care about the time when animation reaches the endpoint; There are no side effect for not calling onControlBarAnimationFinished() when the animation changes, since it will be eventually be called by the new animation.

Comment 26

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #25)
> When the animation is canceled, the promise will be rejected. We only care
> about the time when animation reaches the endpoint; There are no side effect
> for not calling onControlBarAnimationFinished() when the animation changes,
> since it will be eventually be called by the new animation.

OK, but then we should probably catch the promise rejection in this code, and make it a no-op? Otherwise this will cause uncaught promise rejections to show up in tests and/or error telemetry, I think.
Comment on attachment 8964091 [details]
Bug 1449532 - Part III, Polyfill Web Animation API features

https://reviewboard.mozilla.org/r/232886/#review238790

> This calls the handler synchronously from the event handler. That's not how promise resolution .then() callbacks normally work (ie the scheduling is different). I don't think it really matters for this code, especially as you're already having to hack around the other bug here, but it may be worth a comment, as we'll need to check this more carefully when we do ship the full implementation, and it has the potential of hiding bugs on nightly until code hits beta/devedition... so that's a bit unfortunate.
> 
> Perhaps it would be worth using the polyfill even on builds where `animation.finished` support is present? That would avoid nasty surprises when merging beta. I realize it's not ideal, but equally the overhead of the polyfill in this case is very minimal so it doesn't seem too big a deal, so probably worth it to avoid the risk of beta/release-only bugs?

I am aware of the synchronously call from the polyfill. It shouldn't matter since the handleEvent function is invoked directly by event dispatch, hence itself is its own macro task. Wrapping a real Promise here makes it more complex than it needs to. I will make a note about that.

I agree that we should be using the polyfill even on builds where animation.finished support is present.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

a year ago
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6e628dc585f
Part I, Backed out changeset 99fc41ec7ce9 (Bug 1444489 Part VIII) r=Gijs
https://hg.mozilla.org/integration/autoland/rev/62e9d302879c
Part II, Use Web Animation API to animate video control transition r=Gijs
https://hg.mozilla.org/integration/autoland/rev/64486670492f
Part III, Polyfill Web Animation API features r=Gijs

Comment 38

a year ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/c6e628dc585f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Updated

a year ago
Depends on: 1452342

Updated

a year ago
Depends on: 1452926
Depends on: 1486278

Updated

7 months ago
Depends on: 1493089
You need to log in before you can comment on or make changes to this bug.