702894 - Remove pre-DOM API full-screen video implementation (using esc to exit full screen video from built-in player breaks player)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Christopher Blizzard (:blizzard)]

1. Load the attached URL
2. Right click to go full screen
3. Hit [ESC]

Then the video dims and there's a large X in the middle of the screen.  If you mouse click the X in the upper right hand corner the video goes away, but you're left with a spinner in the middle of the video.  Something isn't right in the event handling or internal controls.


Tested on a 9 bet on Windows.

http://hg.mozilla.org/releases/mozilla-beta/rev/31302afe89b3

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

That's using the existing video full-screen code path (not the DOM full-screen API). Once we switch to using the DOM full-screen API for the video controls to go full-screen, ESC won't cancel the video load.

We should keep the fallback code around, so we should preventDefault() on ESC key press in the old video full-screen code.

Comment 2

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

As cpearce said, we should add an event handler for the ESC key and use preventDefault().

The changes should be made in /browser/base/content/fullscreen-video.xhtml

Comment 3

[Christopher Blizzard (:blizzard)]

Is this bug in 10?  We should fix it in 9, I suspect, which is already in beta.

Comment 4

[VD]

HI I am interested in working with this bug! Could you guide me on how to proceed? I am a new contributor here! Thanks!

Comment 5

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

Hi VD, thanks for taking this bug. Now that the fullscreen API is enabled by default, to work on this you should disable the fullscreen API by going to about:config and setting the full-screen-api.enabled to false.

I've placed steps to fix this bug in comment #2. Please let me know if I should explain more.

Comment 6

[Simona Badau, Desktop QA]

Issue is also reproducible on Firefox 10 ESR:

Mozilla/5.0 (Windows NT 6.0; rv:10.0) Gecko/20100101 Firefox/10.0

Comment 7

[VD]

Hi jaws! I have been trying to contact you this last week, but due to timezone differences i never catch you online! could you tell me when you will be free so i could catch you on irc?

Comment 8

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

(In reply to VD from comment #7)
> Hi jaws! I have been trying to contact you this last week, but due to
> timezone differences i never catch you online! could you tell me when you
> will be free so i could catch you on irc?

Hi VD. I apologize but I am on vacation/traveling this week so I may be slow to respond to emails.

Here is the steps to get started on this bug:
1) Go to about:confing and set full-screen-api.enabled to false
2) Open /browser/base/content/fullscreen-video.xhtml
3) Add a keydown event handler and check for the ESC key being the key that is pressed.
4) If the ESC key is being pressed, call event.preventDefault().

Let me know if that is not clear or if you would like more help.

Comment 9

[VD]

HI! I was thinking about adding it to the already existent keypress handler! would that make a difference? 

DOM_VK_ESCAPE is the Esc key right? 

Also my main problem is I have Firefox 9 built in Ubuntu and Firefox 9 on windows vista! but I  am unable to reproduce this bug in either! It seems to work fine! I click on the link, right click on video and go to full screen and press esc, It comes out of full screen! There is no fading or X mark in the centre in this case! 

On Ubuntu sometimes when I press Esc without going on full screen, I get a faded video with a X mark in the centre! 

Could you help me clarify if I have understood the bug!

Comment 10

[VD]

Also could you explain step 1! How do I go to about:config?

Comment 11

[Dão Gottwald [:dao]]

We should just remove fullscreen-video.xhtml now that we shipped the full-screen API.
VD, do you want to do that?

Comment 12

[VD]

Yup sure! Just remove the entire file right? Could you see Comment 9 and tell me if I have understood the question correctly? So it would be easy to check if the issue is sorted!

Thanks!

Comment 13

[Dão Gottwald [:dao]]

It's more work than just removing fullscreen-video.xhtml. The code referencing that file also needs to be updated: http://mxr.mozilla.org/mozilla-central/search?string=fullscreen-video.xhtml&find=browser

With that being done, you don't need to do anything else for this bug, as it only exists within fullscreen-video.xhtml.

Comment 14

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

This fallback is still used when <video> is inside of an iframe that doesn't have the mozallowfullscreen attribute.

If we remove fullscreen-video.xhtml, we will also need to update the nsContextMenu.js to either disable the fullscreen context menuitem or remove it from the context menu in this case.

Comment 15

[VD]

fullScreenVideo: function () {
    let video = this.target;
    if (document.mozFullScreenEnabled)
      video.mozRequestFullScreen();
    else {
      // Fallback for the legacy full-screen video implementation.
      video.pause();
      openDialog("chrome://browser/content/fullscreen-video.xhtml",
                  "", "chrome,centerscreen,dialog=no", video);
    }
  },

yup! exactly wat i was about to ask!

Comment 16

[Dão Gottwald [:dao]]

'document' is the chrome document there. I don't see how document.mozFullScreenEnabled would be false if the video element is in an iframe.

Comment 17

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

Oh sorry, you're right. I forgot about bug 691947 which relaxed the security constraints if the request came from chrome.

Comment 18

[VD]

So shall i go ahead and remove the fullscreen-video.xhtml file and file references? 
and in this case (Comment 15) shall i remove openDialog completely or should i replace it with some other file?

Comment 19

[Dão Gottwald [:dao]]

(In reply to VD from comment #18)
> So shall i go ahead and remove the fullscreen-video.xhtml file and file
> references? 

yep

> and in this case (Comment 15) shall i remove openDialog completely or should
> i replace it with some other file?

just remove it

Comment 20

[VD]

Attachment: Patch for BUg 702894

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 595373 [details] [diff] [review]
Patch for BUg 702894

>   fullScreenVideo: function () {
>     let video = this.target;
>     if (document.mozFullScreenEnabled)
>       video.mozRequestFullScreen();
>     else {
>       // Fallback for the legacy full-screen video implementation.
>       video.pause();
>-      openDialog("chrome://browser/content/fullscreen-video.xhtml",
>-                  "", "chrome,centerscreen,dialog=no", video);
>     }

Please remove the whole else block.

You need to also execute 'hg rm browser/base/content/fullscreen-video.xhtml' before creating the diff.

Comment 22

[VD]

Attachment: Patch for BUg 702894

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 595379 [details] [diff] [review]
Patch for BUg 702894

Thanks!

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c047f38d9381

Comment 25

[Ed Morley [:emorley]]

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

Thank you for the patch! :-)

Comment 26

[Simona Badau, Desktop QA]

This is still reproducible on Firefox 10.0.5 ESR:
Mozilla/5.0 (Windows NT 6.0; rv:10.0.5) Gecko/20100101 Firefox/10.0.5

Is this going to be fixed on the esr branch also?

Comment 27

[Dão Gottwald [:dao]]

(In reply to Simona B [QA] from comment #26)
> Is this going to be fixed on the esr branch also?

No.