1350191 - Video doesn't play normally when I change volume

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[684sigma]

I have a problem with Firefox Beta 53. It doesn't happen in Beta 52 and ESR 45.
Sometimes when I change volume in video, it doesn't play. Here's how to reproduce it:

1. Open https://www.w3schools.com/html/HTML5_video.asp
2. Play the video
3. Click near the beginning of timeline, press down/up several times

Result: video temporarily stops
Expected: video should continue playing

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

An audio channel issue?

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: UX regression, Video playback is interrupted due to volume keyboard shortcut

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ec7fb4f14d3ec23ded7eea40ff49ebbcbec6bde1&tochange=8d2eecb7ea5a16e02862dd326ce4519082ce9901

Regressed by: Bug 1271765


I think the very problematic Bug 1271765 should be backed out from 53 beta.

Comment 3

[Alice0775 White]

affect all video of https://videos.cdn.mozilla.net/uploads/mozillaorg/

Comment 4

[Ray Lin[:ralin]]

Attachment: Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

We could not avoid controls being focused after De-XUL, in order
to preventDefault before event propagate to focused control, we should
change the way of event propagation in media controls.

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

Comment 5

[Ray Lin[:ralin]]

Thanks for report.

This is another derived issue from focusibility after media controls De-XUL. The problem here is the input[type=range] get focused by first click, so up/down arrow key handler would propagate from inner to outer element which fires the native behavior of input[type=range] first and .stopPropagation latter (in current bubble propagation). I've made a patch change the way to "capture" mode to preventDefault() for focused element inside <videocontrols>.

Comment 6

[Ray Lin[:ralin]]

s/key handler/key events/

Comment 7

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

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

https://reviewboard.mozilla.org/r/123564/#review126412

Can you please add a test for this?

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

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

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

https://reviewboard.mozilla.org/r/123564/#review126412

test added :) Thank you for the review.

Comment 10

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

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

https://reviewboard.mozilla.org/r/123564/#review126878

Comment 11

[Ray Lin[:ralin]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e61008f56969

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85866d2aa9f5
Change keypress event propagation to capture to correctly preventDefault if control is focused. r=jaws

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85866d2aa9f5

Comment 14

[Marcia Knous [:marcia]]

Tracking for regression for 53/54/55+.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift to aurora and beta if you think the fix is safe for 53? Thanks.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Alice, about your suggestion in comment 2, if we can fix the issues we know about now, and uplift them to 53 beta, I'm hoping we may still be ok for this to move to release. I asked for more evaluation and feedback in bug 1271765.

Comment 17

[Ray Lin[:ralin]]

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1271765
[User impact if declined]: pressing up/down arrow key triggers video seek if timeline has just been clicked by user.
[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]: better to have, same as comment 0. 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: Media shortcuts works as-is. This patch only prevents keypress event from propagating inside video.
[String changes made/needed]: none

Thanks!

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

fix an issue with keypress propagation in video controls, aurora54+

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3eb3bc65705e

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8851191 [details]
Bug 1350191 - Change keypress event propagation to capture to correctly preventDefault if control is focused.

Fix for minor regression in a/v controls, includes new tests. Let's take this for beta 9.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9069793e7750

Comment 22

[Bogdan Maris, Desktop Test Engineering]

I reproduced the initial issue on Firefox 53 beta 8, verified that the issue is fixed using Firefox 53 beta 9, latest Developer Edition 54.0a2 and latest Nightly 55.0a1 across platforms (Windows 10 64bit, macOS 10.12.2 and Ubuntu 16.04 32bit).