1348601 - Context menu 'play' control for HTML5 video initially fails to work when autoplay is disabled

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Finn Prideaux]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829

Steps to reproduce:

1. Disable autoplay of HTML5 media by setting media.autoplay.enabled to false in about:config.
2. Using Firefox, navigate to a page with an embedded HTML5 video that does not have custom video controls (e.g. http://i.imgur.com/A76wD5p.gifv)
3. Right-click to bring up the context menu, and click 'Play'.


Actual results:

Video fails to play when 'Play' is clicked in the context menu.

In order to play the video, it is necessary to select 'Show controls', and then use the play control that pops up on the video. After doing this, the context-menu 'Play' and 'Pause' controls will work.


Expected results:

Video should play when 'Play' is selected from the context menu. (This was the behaviour prior to Firefox 52.)

Comment 1

[Finn Prideaux]

(Tested in a new profile with no extensions.)

Comment 2

[Loic]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1a3194836cb4c3da6ba3a9742a2d25cf26669b55&tochange=90b691bf09f5cc4fe7d0c6445fcf5afa2c34eeee

Kilik Kuo — Bug 1316758-Remove nsContentUtils::LegacyIsCallerChromeOrNativeCode and IsCallerChrome calls from HTMLMediaElement. r=cpearce

Kilik, could you check this regression from your patch, please.

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

It's a regression.

After modification to this line [1], HTMLME is not able to identify 'play()' called from chrome, here, the case is from context menu. So even this is a call by user's interaction, the code path still returns here [2] resulting a rejected promise.

EventStateManager::IsHandlingUserInput() will be true if user is clicking the play icon from control, but it will be false if user is clicking from context menu.
I'd like to sync the behavior above in order to not calling |nsContentUtils::IsCallerChrome()| again.

[1] https://hg.mozilla.org/integration/autoland/rev/d6b94b6c5fb0#l1.75
[2] http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/dom/html/HTMLMediaElement.cpp#3868-3871

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

The problem is that the information of IsHandlingUserInput for EventStateManager cannot be carried through mm.SendAsyncMessange [1] to the message handler in content.js [2].


[1] http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/browser/base/content/nsContextMenu.js#1744
[2] http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/browser/base/content/content.js#796

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

HTMLMediaElement relies on user's interaction to allow playback when media.autoplay.enabled set to false.

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

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

https://reviewboard.mozilla.org/r/122268/#review124652

::: browser/base/content/content.js:831
(Diff revision 1)
>  <span class="indent">>>>></span>    case "fullscreen":
>  <span class="indent">>>>></span>      if (content.document.fullscreenEnabled)
>  <span class="indent">>>>></span>        media.requestFullscreen();
>  <span class="indent">>>>></span>      break;
>  <span class="indent">>>>></span>  }
> +    })

Nit: add a semicolon to the end of this line.

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

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

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

https://reviewboard.mozilla.org/r/122268/#review124652

> Nit: add a semicolon to the end of this line.

Thanks !

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abad919f22b0
Carry IsHandlingUserInput information to media listeners in content.js. r=mrbkap

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

The patch is also applicable for aurora / beta.
But I'm not sure if this is necessary to be uplifted to release.
Gerry, any suggestions from Release Manager ?

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1316758
[User impact if declined]: Media-related operations on ContextMenu won't work correctly when pref "media.autoplay.enabled" is set to false.
[Is this code covered by automated tests?]: no, as far as I can tell, only context menu layout is covered.
[Has the fix been verified in Nightly?]: yes, on my local pc.
[Needs manual test from QE? If yes, steps to reproduce]: yes
Step 1. Go about:config, and set media.autoplay.enabled to false
Step 2. Go http://i.imgur.com/A76wD5p.gifv, video should not start playback.
Step 3. Right click on the video element, context menu pops then click media-related buttons.
Step 4. Check if operation works.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: little
[Why is the change risky/not risky?]: The utility function to carry IsHandlingUserInput information was already landed and is used for a while, it's also used somewhere in nsContextMenu.js.
[String changes made/needed]: None.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abad919f22b0

Comment 12

[Marcia Knous [:marcia]]

Tracking 53/54/55+. I think this is a good candidate to take for aurora/beta.

Comment 13

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 14

[Julien Cristau [:jcristau]]

Doesn't seem worth taking in a dot release.

Comment 15

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

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

Regression from 52, verified in m-c, let's take this for aurora and beta. 
The fix should be in 53 beta 7 next week.

Comment 16

[Brindusa Tot, Desktop QA]

Reproduce this on Nightly 54.0a1, Build ID 20170302030206 on Windows 10 x64.

I can confirm the fix on the latest Nightly 55.0a1, build ID 20170324030205 on Windows 10 x64, Ubuntu 16.04 x 64 and Mac OS X 10.12.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97181d67a50b
https://hg.mozilla.org/releases/mozilla-beta/rev/991a3145ada2

Comment 18

[Lina Butler [:lina]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97181d67a50b

Comment 19

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Verified as fixed using Firefox 53 beta 7 and latest Aurora 54.0a2 2017-03-27 under Win 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.10.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

Grafts cleanly to ESR52. Verified fixed across all branches it's on with no known regressions.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8849484 [details]
Bug 1348601 - Carry IsHandlingUserInput information to media listeners in content.js.

regression in 52, let's fix this in 52.1esr

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1848bd238064

Comment 23

[Andrei Vaida [:avaida]]

Petruta, could you please verify this on 52.1esr as well?

Comment 24

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Verified as fixed on Firefox 52.1esr under Win 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.