Bug 138717 – [Media] Timeline scrubber not updating as the video plays

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

[Media] Timeline scrubber not updating as the video plays

[Dean Jackson]

Created attachment 241519 [details]
Patch

[Jer Noble]

Dino, I don't think this is the right fix. The problem is that the controls aren't updating when they go from hidden to showing; instead, all the deferred updates need to be performed at that time.

[Dean Jackson]

(In reply to comment #2)
> Dino, I don't think this is the right fix. The problem is that the controls
> aren't updating when they go from hidden to showing; instead, all the
> deferred updates need to be performed at that time.

The problem is that there are five states:

- hidden
- show
- paused
- :hover
- no CSS class (which is kind-of a hidden, but not with display none)

(Note: this is fairly confusing, not only because the tense changes in the state names :), but because the hover state is not able to be queried from script easily)

I think we can simplify this into a single "visible" state (although I'd probably call it "hidden"). I'm not sure we need the existing "hidden" - the display: none doesn't buy us much if we're smart about not updating the controls all the time.

You're right that the controls don't update when they become visible. I think we should add that in showControls(), although I think it would need to go in the deferred call to timeline metrics update (or that should also update the controls). The reason this current patch passes the test is that it gets an event from the media which triggers the scrubber update.

[Dean Jackson]

Created attachment 241602 [details]
Patch

[Jer Noble]

Comment on attachment 241602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241602&action=review

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1038
> +        this.updateTime();

You need to call this.updateProgress() here too.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1053
> -        return !this.isAudio() && (!this.controls.panel.classList.contains(this.ClassNames.show) || this.controls.panel.classList.contains(this.ClassNames.hidden));
> +        return !this.isAudio() && this.controls.panel.classList.contains(this.ClassNames.hidden);

With this change, the controls will continue to update when they are hidden due to inactivity during playback. Is this what you want?

[Dean Jackson]

(In reply to comment #5)
> Comment on attachment 241602 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241602&action=review
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1038
> > +        this.updateTime();
> 
> You need to call this.updateProgress() here too.

Yeah.

> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1053
> > -        return !this.isAudio() && (!this.controls.panel.classList.contains(this.ClassNames.show) || this.controls.panel.classList.contains(this.ClassNames.hidden));
> > +        return !this.isAudio() && this.controls.panel.classList.contains(this.ClassNames.hidden);
> 
> With this change, the controls will continue to update when they are hidden
> due to inactivity during playback. Is this what you want?

That's probably ok, since the updates would be fairly rare. e.g. we're not getting the playback tick events.

[Dean Jackson]

Created attachment 241608 [details]
Patch

[Jer Noble]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 241602 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=241602&action=review
> > 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1038
> > > +        this.updateTime();
> > 
> > You need to call this.updateProgress() here too.
> 
> Yeah.
> 
> > 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1053
> > > -        return !this.isAudio() && (!this.controls.panel.classList.contains(this.ClassNames.show) || this.controls.panel.classList.contains(this.ClassNames.hidden));
> > > +        return !this.isAudio() && this.controls.panel.classList.contains(this.ClassNames.hidden);
> > 
> > With this change, the controls will continue to update when they are hidden
> > due to inactivity during playback. Is this what you want?
> 
> That's probably ok, since the updates would be fairly rare. e.g. we're not
> getting the playback tick events.

No, the opposite. The controls are hidden during playback, so you'd be getting many, many playback tick events.

[Dean Jackson]

(In reply to comment #8)

> > > With this change, the controls will continue to update when they are hidden
> > > due to inactivity during playback. Is this what you want?
> > 
> > That's probably ok, since the updates would be fairly rare. e.g. we're not
> > getting the playback tick events.
> 
> No, the opposite. The controls are hidden during playback, so you'd be
> getting many, many playback tick events.

Sorry, I misunderstood your comment.

I'm pretty sure they don't, for a couple of reasons:

- I turned on repaint counters and didn't see painting while the controls were hidden

- As we transition to the hidden state, the opacity transition causes handlePanelTransitionEnd, which adds the hidden class. controlsAreHidden() tests for hidden.

So we'll update as we are fading out (which would be correct) but not once we've actually disappeared.

[Jer Noble]

Comment on attachment 241608 [details]
Patch

(In reply to comment #9)
> (In reply to comment #8)
> 
> > > > With this change, the controls will continue to update when they are hidden
> > > > due to inactivity during playback. Is this what you want?
> > > 
> > > That's probably ok, since the updates would be fairly rare. e.g. we're not
> > > getting the playback tick events.
> > 
> > No, the opposite. The controls are hidden during playback, so you'd be
> > getting many, many playback tick events.
> 
> Sorry, I misunderstood your comment.
> 
> I'm pretty sure they don't, for a couple of reasons:
> 
> - I turned on repaint counters and didn't see painting while the controls
> were hidden
> 
> - As we transition to the hidden state, the opacity transition causes
> handlePanelTransitionEnd, which adds the hidden class. controlsAreHidden()
> tests for hidden.
> 
> So we'll update as we are fading out (which would be correct) but not once
> we've actually disappeared.

Okay, r=me.

[Dean Jackson]

Committed r176139: <http://trac.webkit.org/changeset/176139>