Closed Bug 1449532 Opened 6 years ago Closed 6 years ago

WebVTT subtitle does not reposition after control bar fades

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(3 files)

Bug 1449532 - Part I, Backed out changeset 99fc41ec7ce9 (Bug 1444489 Part VIII)
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1449532 - Part II, Use Web Animation API to animate video control transition
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1449532 - Part III, Polyfill Web Animation API features
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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
Attachment #8963133 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs)
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 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 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.
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!
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs)
Attachment #8964091 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963134 - Flags: review?(gijskruitbosch+bugs)
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 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.
(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.
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
https://hg.mozilla.org/mozilla-central/rev/c6e628dc585f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1452342
Depends on: 1452926
Depends on: 1486278
Depends on: 1493089
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: