1239372 - The tab sound indicator should not be hiding when using mouse clicking to seek the video

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Alastor Wu [:alwu]]

STR.
1. Go to https://www.youtube.com/watch?v=cxeQmF4sUJY
2. Seek to arbitrary position

Expected.
3. Sound icon shouldn't disappear during seeking

Actual.
3. Sound icon disappear during seeking

Comment 1

[Alastor Wu [:alwu]]

In this case, this process would trigger MediaElement::Pause() before the MediaElement::Seek(), so the problem happen.

However, this behavior isn't equal to every MediaElement. I tried to seek another audio element [1], and I found that the MediaElement::Seek() was triggered before MediaElement::Seek(). 

Here are two things we need to get them more clear, 
First, do we need to call Pause() when seeking the video?
Second, what's the correct behavior when seeking the MediaElement?

[1] http://people.mozilla.org/~alwu/Test.html

Comment 2

[arni2033]

1) Your Step 2 isn't precise enough. Does "seek" means "click on progressbar" or "hover mouse over
   progressbar, hold left mouse button, then move mouse pointer within progressbar, then
   release left mouse button"? Or maybe "Hold Right arrow on the keyboard"?
   Actually, in all 3 cases producing sound is undesired option and therefore
2) The "Actual" result in comments 0-1 is completely expected, since nothing is causing sound. It's
   not clear why you're calling a problem the sound indicator being hidden when there's no sound

See also bug 1225000

Comment 3

[Alastor Wu [:alwu]]

Hi, Arni,

In this issue, the case I want to discuss here is "click on progressbar". The sound icon should only disappear when the second case you mentioned happen (hover mouse over progressbar...) Another interesting point is the third case (Hold Right arrow on the keyboard) doesn't let the sound icon disappear, and it's exactly the ideal behavior in my mind.

However, IMMO, the sound icon shouldn't disappear in rest of these cases. Seeking is a transition behavior, it happens during very short period, we should keep the sound icon state as same as the state before seeking. 

If you think the sound icon need to disappear during this very short period, that means the users would see the icon blinking when they watch the video with a lots of short silent. In addition, the UX suggest is in [1]. It said that we only need to handle the silence longer than 10 seconds. The bug 1225000 should also follow the 10-seconds rule.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1235612#c18

Comment 4

[Alastor Wu [:alwu]]

Hi, Philipp,
Sorry to bother you, need your help again!
Could you help me verify whether the sound icon should be displayed or not in the following situations?

* pre-condition : Users are watching an audible video, and then they want to seek video *

Here are three different kinds of seeking.
(1) Click on the specific position on the progress-bar, they want the video start playing from that 
    position.
(2) Hover mouse over progress-bar, hold left mouse button, then move mouse pointer within progress-bar, 
    then user can find the position they want from the preview image, release left mouse button until they
    decide where the video should start playing from.
(3) Hold left/right arrow on the keyboard, to decrease/increase a fixed-interval for position.

Very appreciate :)

Comment 5

[arni2033]

I see now, you only want to reduce blinking in this bug. However, messing with timeouts doesn't look like a good idea:  If in scenario (2) I move mouse about 10.1-10.5 seconds and then release left mouse button, then I will still see blinking "visible -> invisible -> visible"
Also, this was already requested in bug 1232357. That's really ~5 lines of CSS to make a transition.

Anyway, here's a few notes about comments 3-4:
(1) in comment 4 is subset of (2), so this somehow reduces the number of cases...
Scenario (3) actually doesn't hide the sound indicator, yes. That's because it doesn't stop the video!
And sometimes I hear some short sounds during seeking. Video/audio element can't tell the difference between "press" and "hold", and that sounds like a separate bug (enhancement?) to me.

Comment 6

[Alastor Wu [:alwu]]

I like the idea of the bug1232357, if we can do this kinds of fading animation that we wouldn't worry about the icon blinking.

If so, I think this issue should be solved in the front-end side, we would still dispatch the "audio-playback-false" event when the audio pausing or no any audible sound. However, the front-end side need to handle the logic about icon disappearing (or fading).

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alastor Wu [:alwu] from comment #4)
> Hi, Philipp,
> Sorry to bother you, need your help again!
> Could you help me verify whether the sound icon should be displayed or not
> in the following situations?
> 
> * pre-condition : Users are watching an audible video, and then they want to
> seek video *
> 
> Here are three different kinds of seeking.
> (1) Click on the specific position on the progress-bar, they want the video
> start playing from that 
>     position.
> (2) Hover mouse over progress-bar, hold left mouse button, then move mouse
> pointer within progress-bar, 
>     then user can find the position they want from the preview image,
> release left mouse button until they
>     decide where the video should start playing from.
> (3) Hold left/right arrow on the keyboard, to decrease/increase a
> fixed-interval for position.
> 
> Very appreciate :)

Hm, if I understand you correctly, all of the scenarios depend upon whether or not sound is played during seeking. If the page keeps playing audio while seeking: keep showing the indicator. If not: wait for the timeout and if it still isn't playing audio after that: hide it.

I also like the idea of fading the indicator in and out. That would make things a lot smoother. It might not be possible though, because the indicator is changing the size of the tab label, and I don't think we can animate that easily.

Comment 8

[Alastor Wu [:alwu]]

I would modify our native video-control, let the sound icon keeps showing during the seeking with clicking.

So the STR is like following,
STR.
1. Go to http://people.mozilla.org/~alwu/Test.html
2. Play video
3. Use mouse to click on the specific position on the progress-bar, and then release mouse

Expected.
4. Sound icon shouldn't disappear during seeking

Actual.
4. Sound icon disappear during seeking

Comment 9

[Alastor Wu [:alwu]]

For youtube, they have their own video-control-interface, so we can't control it.

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1239372 - only pause video during draging.

Review commit: https://reviewboard.mozilla.org/r/35231/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35231/

Comment 11

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9633c8543e38

Comment 12

[Alastor Wu [:alwu]]

Hi, Jared,
Sorry to bother you, could you help me review this patch?
Very appreciate :)

---

The main idea of this patch is to prevent the tab sound indicator flickering when users use clicking to seek the video. The reproduce steps is in the comment8.
I also list the possible seeking behaviors in comment4, we only need to call video.pause in the case (2), when user is continually dragging the progress-bar and doens't trigger mouse-up.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Hi Alastor, thanks for the patch. Fixing it in the video controls will only make the experience better for HTML5 media that uses our default controls, but not sites like YouTube that provide their own controls.

A better fix would be bug 1232357 which will work everywhere and should cover the case of seeking within a video.

Comment 14

[Alastor Wu [:alwu]]

Ok, thanks :)

Comment 15

[Alastor Wu [:alwu]]

Hi, Jared,
I think we still need that because on Fennec we use the default control interface, the media control [1] would be flashing if we don't fix this issue. The bug 1232357 only affects on the desktop.
How do you think?
Thanks!

Comment 16

[Alastor Wu [:alwu]]

FYI, Youtube uses our default control interface on Fennec.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

I didn't think of that before. I'm a bit worried about not pausing during dragging and what other types of bugs we may introduce due to the amount of state that we store in the videocontrols binding.

I think a more robust fix, and one that would also fix bug 1232357 would be to change the lower level code that announces audio has stopped. That code should have the 3 second delay built in to it.

Comment 18

[Alastor Wu [:alwu]]

Hi, Jaren,
Sorry for the late reply.
Could you help me explain the potential side affect if we do seeking before pausing?

IMMO, the platform audio notification should be reflected the audio playing state precisely. If we want to do some delay, it might be implemented in front-end code for each different platform.

Eg. The media control interface on Fennec is also depend on the platform audio notification to decide when it should be show/hide. And I don't want to delay the hiding time for that.

Thanks!

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8720215 [details]
Bug 1239372 - only pause video during draging.

https://reviewboard.mozilla.org/r/35231/#review56302

Okay, let's go with this for now and see what comes out of it. The code looks okay and not too different than what we were doing before besides the behavioral change.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Alastor Wu [:alwu] from comment #18)
> Hi, Jaren,
> Sorry for the late reply.
> Could you help me explain the potential side affect if we do seeking before
> pausing?

We have had bugs before about the videocontrols making some assumptions about the internal state of the video due to some other state being cached by the videocontrols. I'm not sure if this will cause any of those but we can try this out to see. It looks sound to me.
 
> IMMO, the platform audio notification should be reflected the audio playing
> state precisely. If we want to do some delay, it might be implemented in
> front-end code for each different platform.
> 
> Eg. The media control interface on Fennec is also depend on the platform
> audio notification to decide when it should be show/hide. And I don't want
> to delay the hiding time for that.

Yes, you are right. We shouldn't lie about when the audio stops. The delay should remain a front-end feature.

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8720215 [details]
Bug 1239372 - only pause video during draging.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35231/diff/1-2/

Comment 22

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d73ea18ec2

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cef62921024
only pause video during draging. r=jaws

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cef62921024

Comment 25

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 46.0a1 (Build ID:20160113030208) on 
Windows 8.1, 64-bit.

Verified as fixed with Firefox Nightly 50.0a1 (Build ID: 20160713030216)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 26

[Subhrajyoti Sen]

Verified as fixed on Firefox Aurora 50.0a2(2016-09-09) using Linux Mint 18.

Comment 27

[Kumaraguru]

[testday-20160909]

Comment 28

[Moin Shaikh]

Verified on FF 50.3beta

[testday-20160930]

Comment 29

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Based on comment 25, comment 26 and comment 28, I will set the corresponding flags.