Closed Bug 1352724 Opened 7 years ago Closed 7 years ago

Seeking in video unexpectedly starts the playback, and video controls don't display current time

Categories

(Toolkit :: Video/Audio Controls, defect)

Product:
Toolkit
Component:
Video/Audio Controls
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: 684sigma, Assigned: ralin)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1352724 - Pause video at the point when scrubber is being click.
59 bytes, text/x-review-board-request
jaws
: review+
jcristau
: approval-mozilla-beta-
Details
Bug 1352724 - Fix the indentation and spelling error.
59 bytes, text/x-review-board-request
jaws
: review+
jcristau
: approval-mozilla-beta-
Details 
I have a problem with Firefox Beta 52. It also happens in Nightly 55. Doesn't happen in ESR 45.
Sometimes when I click in video timeline and think whether it's the right part of the video or not, playback continues unexpectedly. Video controls don't update and display incorrect time (the time under mouse cursor). When I release the mouse button, video controls update, and shows the actual time.
Here's how to reproduce the bug:

1. Open https://www.w3schools.com/html/mov_bbb.mp4
2. Click (hold mouse button) near the beginning of timeline

Result: Playback continues, but currentTime in timeline doesn't change, it's right end is right under the mouse cursor
Expected: Playback shouldn't continue
Has STR: --- → yes
Keywords: regression
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=87aee9d5e04dabf5d5fdce4617df454c88fb0110&tochange=8cef6292102420b8a144c2752ddd96c906b6c7be

Regressed by: 8cef62921024	Alastor Wu — Bug 1239372 - only pause video during draging. r=jaws
Blocks: 1239372
Status: UNCONFIRMED → NEW
status-firefox52: --- → wontfix
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → wontfix
Ever confirmed: true
Version: 52 Branch → 50 Branch
Alastor, can you look into this regression?
Flags: needinfo?(alwu)
Sorry, now I'm busy with other stuffs and I'm afraid of not have enough time to do it.

---

Hi, Ray,
Do you have any interest to take this bug?
Thanks!
Flags: needinfo?(alwu) → needinfo?(ralin)
Feel free to transfer NI back to me if you don't have time.
Taking this bug :)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Flags: needinfo?(ralin)
Given that bug 1232357 has been landed, we don't need to care about and to distinguish the point of "isDragging". So, I'd make scrubber behave like Chrome does(or we used to do?): pause the video right after click, and resume after mouseup.
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

https://reviewboard.mozilla.org/r/133832/#review136886

The change looks good but we should have a test for this.
Attachment #8861826 - Flags: review?(jaws) → review-
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

https://reviewboard.mozilla.org/r/133832/#review137736

::: toolkit/content/tests/widgets/test_videocontrols.html:416
(Diff revision 2)
> +      ok(true, "video position is at " + video.currentTime);
> +      SimpleTest.finish();

Please add another case here for the video resuming play after the "seeked" event.
Attachment #8861826 - Flags: review?(jaws) → review-
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

https://reviewboard.mozilla.org/r/133832/#review140610

::: toolkit/content/tests/widgets/test_videocontrols.html:417
(Diff revisions 2 - 3)
>        break;
>  
>      case 32:
>        is(event.type, "seeked", "checking event type");
>        ok(true, "video position is at " + video.currentTime);
> +      synthesizeMouse(video, scrubberOffsetX + 10, scrubberCenterY, { type: "mouseup",   button: 0 });

We shouldn't need to specify the type of the event or which button. By default, just calling
> synthesizeMouse(video, scrubberOffsetX + 10, scrubberCenterY, { });

should be enough.

::: toolkit/content/widgets/videocontrols.xml:522
(Diff revisions 2 - 3)
>        SHOW_THROBBER_TIMEOUT_MS: 250,
>        _showThrobberTimer: null,
>        _delayShowThrobberWhileResumingVideoDecoder() {
>            this._showThrobberTimer = setTimeout(() => {
>                this.statusIcon.setAttribute("type", "throbber");
> -              this.setupStatusFader();
> +              // Show the throbber immediatelly since we have waited for SHOW_THROBBER_TIMEOUT_MS.

spelling error, "immediately" instead of "immediatelly"

::: toolkit/content/widgets/videocontrols.xml:524
(Diff revisions 2 - 3)
> +              // transition-duration(300ms).
> +              this.setupStatusFader(true);
>            }, this.SHOW_THROBBER_TIMEOUT_MS);
>        },
>        _cancelShowThrobberWhileResumingVideoDecoder() {
>            if (this._showThrobberTimer) {
>              clearTimeout(this._showThrobberTimer);
>              this._showThrobberTimer = null;

Please fix the indent of these two functions to have two-space indentation like the rest of this file. This indentation fix should be in a separate commit.
Attachment #8861826 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Comment on attachment 8861826 [details]
> Bug 1352724 - Pause video at the point when scrubber is being click.
> 
> https://reviewboard.mozilla.org/r/133832/#review140610
> 
> ::: toolkit/content/tests/widgets/test_videocontrols.html:417
> (Diff revisions 2 - 3)
> >        break;
> >  
> >      case 32:
> >        is(event.type, "seeked", "checking event type");
> >        ok(true, "video position is at " + video.currentTime);
> > +      synthesizeMouse(video, scrubberOffsetX + 10, scrubberCenterY, { type: "mouseup",   button: 0 });
> 
> We shouldn't need to specify the type of the event or which button. By
> default, just calling
> > synthesizeMouse(video, scrubberOffsetX + 10, scrubberCenterY, { });
> 
> should be enough.
I thought the STR is holding the mouse click, so we need an opposite to the previous "mousedown" event.



> ::: toolkit/content/widgets/videocontrols.xml:522
> (Diff revisions 2 - 3)
> >        SHOW_THROBBER_TIMEOUT_MS: 250,
> >        _showThrobberTimer: null,
> >        _delayShowThrobberWhileResumingVideoDecoder() {
> >            this._showThrobberTimer = setTimeout(() => {
> >                this.statusIcon.setAttribute("type", "throbber");
> > -              this.setupStatusFader();
> > +              // Show the throbber immediatelly since we have waited for SHOW_THROBBER_TIMEOUT_MS.
> 
> spelling error, "immediately" instead of "immediatelly"
> 
> ::: toolkit/content/widgets/videocontrols.xml:524
> (Diff revisions 2 - 3)
> > +              // transition-duration(300ms).
> > +              this.setupStatusFader(true);
> >            }, this.SHOW_THROBBER_TIMEOUT_MS);
> >        },
> >        _cancelShowThrobberWhileResumingVideoDecoder() {
> >            if (this._showThrobberTimer) {
> >              clearTimeout(this._showThrobberTimer);
> >              this._showThrobberTimer = null;
> 
> Please fix the indent of these two functions to have two-space indentation
> like the rest of this file. This indentation fix should be in a separate
> commit.
It seems this part is mixed with the other patch...have no idea how to avoid that after rebase :\
I'll revise the patch and also fix the indentation in a separate commit.

Thanks :)
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

https://reviewboard.mozilla.org/r/133832/#review140610

Thanks for reviewing,

I've fix the issues, could you help to review again?

> We shouldn't need to specify the type of the event or which button. By default, just calling
> > synthesizeMouse(video, scrubberOffsetX + 10, scrubberCenterY, { });
> 
> should be enough.

It does work as your suggestion, thanks.

> Please fix the indent of these two functions to have two-space indentation like the rest of this file. This indentation fix should be in a separate commit.

Fixed this and spelling error in a separate commit.
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

https://reviewboard.mozilla.org/r/133832/#review145170
Attachment #8861826 - Flags: review?(jaws) → review+
Comment on attachment 8867969 [details]
Bug 1352724 - Fix the indentation and spelling error.

https://reviewboard.mozilla.org/r/139498/#review145172
Attachment #8867969 - Flags: review?(jaws) → review+
Thanks
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f1889f4eda86
Pause video at the point when scrubber is being click. r=jaws
https://hg.mozilla.org/integration/autoland/rev/22562d9f1f91
Fix the indentation and spelling error. r=jaws
Keywords: checkin-needed
Please request uplift on this when you get a chance.  Thanks!
Flags: needinfo?(ralin)
Thanks Nathan,

Since a intermittent(bug 1367194) was introduced by this patch, I'd fix it first and uplift both then.
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1239372
[User impact if declined]: Time label doesn't update when user click&hold on timeline but playback continue in the meanwhile.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: uplift this prior to uplifting Bug 1367194
[Is the change risky?]: low
[Why is the change risky/not risky?]: doesn't break existing seeking test case, and this only simplify the state control while seeking.
[String changes made/needed]: none

Thanks.
Flags: needinfo?(ralin)
Attachment #8861826 - Flags: approval-mozilla-beta?
Ray, are you requesting the two patches to be uplifted or just one?
Thanks
Flags: needinfo?(ralin)
two thanks.
Flags: needinfo?(ralin)
Attachment #8867969 - Flags: approval-mozilla-beta?
Comment on attachment 8861826 [details]
Bug 1352724 - Pause video at the point when scrubber is being click.

seems a bit of a corner case, and not a new regression, so I'd rather not take this late in the beta cycle.
Attachment #8861826 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8867969 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I have reproduce this bug with Nightly 55.0a1 (2017-04-01) (64-bit) in Manjaro Linux, 64bit

This bug is now verified as fixed in latest beta 55.0b1

Build ID 	20170613054006
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [bugday-20170614]
I can reproduce this issue with Nightly 55.0a1 (2017-04-01) on Windows 7, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID 	20170803103124
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170802]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: