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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: blizzard, Assigned: VD)

Tracking

9 Branch
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jwein][lang=js], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
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.
Depends on: 470628
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
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Using esc to exit full screen video from built-in player breaks player → Using esc to exit full screen video from built-in player breaks player in pre-DOM API full-screen implementation
Whiteboard: [good first bug][mentor=jwein][lang=js]

Updated

6 years ago
Component: DOM: Core & HTML → General
Product: Core → Firefox
QA Contact: general → general
(Reporter)

Comment 3

6 years ago
Is this bug in 10?  We should fix it in 9, I suspect, which is already in beta.
(Assignee)

Comment 4

5 years ago
HI I am interested in working with this bug! Could you guide me on how to proceed? I am a new contributor here! Thanks!
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.
Assignee: nobody → vandhanaa91
Status: NEW → ASSIGNED

Comment 6

5 years ago
Issue is also reproducible on Firefox 10 ESR:

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

Comment 7

5 years ago
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?
(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.
(Assignee)

Comment 9

5 years ago
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!
(Assignee)

Comment 10

5 years ago
Also could you explain step 1! How do I go to about:config?
We should just remove fullscreen-video.xhtml now that we shipped the full-screen API.
VD, do you want to do that?
(Assignee)

Comment 12

5 years ago
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!
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.
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.
(Assignee)

Comment 15

5 years ago
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!
'document' is the chrome document there. I don't see how document.mozFullScreenEnabled would be false if the video element is in an iframe.
Oh sorry, you're right. I forgot about bug 691947 which relaxed the security constraints if the request came from chrome.
(Assignee)

Comment 18

5 years ago
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?
(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
(Assignee)

Comment 20

5 years ago
Created attachment 595373 [details] [diff] [review]
Patch for BUg 702894
Attachment #595373 - Flags: review?(jwein)
Attachment #595373 - Flags: review?(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.
Attachment #595373 - Flags: review?(jwein)
Attachment #595373 - Flags: review?(dao)
Attachment #595373 - Flags: feedback+
(Assignee)

Comment 22

5 years ago
Created attachment 595379 [details] [diff] [review]
Patch for BUg 702894
Attachment #595373 - Attachment is obsolete: true
Attachment #595379 - Flags: review?(jwein)
Attachment #595379 - Flags: review?(dao)
Comment on attachment 595379 [details] [diff] [review]
Patch for BUg 702894

Thanks!
Attachment #595379 - Flags: review?(dao) → review+

Updated

5 years ago
Keywords: checkin-needed
Summary: Using esc to exit full screen video from built-in player breaks player in pre-DOM API full-screen implementation → Remove pre-DOM API full-screen video implementation (using esc to exit full screen video from built-in player breaks player)
http://hg.mozilla.org/integration/mozilla-inbound/rev/c047f38d9381
Keywords: checkin-needed
Target Milestone: --- → Firefox 13

Updated

5 years ago
Attachment #595379 - Flags: review?(jwein)
https://hg.mozilla.org/mozilla-central/rev/c047f38d9381

Thank you for the patch! :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 734040
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?
(In reply to Simona B [QA] from comment #26)
> Is this going to be fixed on the esr branch also?

No.
Blocks: 520160
You need to log in before you can comment on or make changes to this bug.