Last Comment Bug 702894 - Remove pre-DOM API full-screen video implementation (using esc to exit full screen video from built-in player breaks player)
: Remove pre-DOM API full-screen video implementation (using esc to exit full s...
Status: RESOLVED FIXED
[good first bug][mentor=jwein][lang=js]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 9 Branch
: All All
: -- normal (vote)
: Firefox 13
Assigned To: VD
:
Mentors:
http://www.0xdeadbeef.com/~blizzard/w...
Depends on: 470628 734040
Blocks: 520160
  Show dependency treegraph
 
Reported: 2011-11-16 00:04 PST by Christopher Blizzard (:blizzard)
Modified: 2013-01-03 14:10 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for BUg 702894 (2.47 KB, patch)
2012-02-08 04:13 PST, VD
dao+bmo: feedback+
Details | Diff | Splinter Review
Patch for BUg 702894 (8.72 KB, patch)
2012-02-08 04:36 PST, VD
dao+bmo: review+
Details | Diff | Splinter Review

Description Christopher Blizzard (:blizzard) 2011-11-16 00:04:42 PST
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) 2011-11-16 00:35:58 PST
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) 2011-11-16 00:51:04 PST
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) 2011-11-16 09:42:10 PST
Is this bug in 10?  We should fix it in 9, I suspect, which is already in beta.
Comment 4 VD 2012-01-29 20:11:25 PST
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) 2012-01-30 01:10:51 PST
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 B [:simonab ] 2012-01-31 05:24:55 PST
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 2012-02-07 04:36:12 PST
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) 2012-02-07 15:06:10 PST
(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 2012-02-07 15:19:58 PST
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 2012-02-07 15:23:43 PST
Also could you explain step 1! How do I go to about:config?
Comment 11 Dão Gottwald [:dao] 2012-02-07 15:26:48 PST
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 2012-02-07 15:47:26 PST
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] 2012-02-07 15:52:36 PST
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) 2012-02-07 16:10:05 PST
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 2012-02-07 16:14:08 PST
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] 2012-02-07 16:21:51 PST
'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) 2012-02-07 16:31:33 PST
Oh sorry, you're right. I forgot about bug 691947 which relaxed the security constraints if the request came from chrome.
Comment 18 VD 2012-02-07 19:23:07 PST
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] 2012-02-08 00:54:03 PST
(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 2012-02-08 04:13:06 PST
Created attachment 595373 [details] [diff] [review]
Patch for BUg 702894
Comment 21 Dão Gottwald [:dao] 2012-02-08 04:22:50 PST
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 2012-02-08 04:36:08 PST
Created attachment 595379 [details] [diff] [review]
Patch for BUg 702894
Comment 23 Dão Gottwald [:dao] 2012-02-08 05:00:14 PST
Comment on attachment 595379 [details] [diff] [review]
Patch for BUg 702894

Thanks!
Comment 25 Ed Morley [:emorley] 2012-02-09 10:35:45 PST
https://hg.mozilla.org/mozilla-central/rev/c047f38d9381

Thank you for the patch! :-)
Comment 26 Simona B [:simonab ] 2012-06-01 08:41:19 PDT
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] 2012-06-01 08:57:26 PDT
(In reply to Simona B [QA] from comment #26)
> Is this going to be fixed on the esr branch also?

No.

Note You need to log in before you can comment on or make changes to this bug.