Closed Bug 702894 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: General, defect)

9 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: blizzard, Assigned: vandhanaa91)

References

()

Details

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

Attachments

(1 file, 1 obsolete file)

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]
Component: DOM: Core & HTML → General
Product: Core → Firefox
QA Contact: general → general
Is this bug in 10? We should fix it in 9, I suspect, which is already in beta.
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
Issue is also reproducible on Firefox 10 ESR: Mozilla/5.0 (Windows NT 6.0; rv:10.0) Gecko/20100101 Firefox/10.0
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.
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!
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?
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.
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.
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
Attached patch Patch for BUg 702894 (obsolete) — Splinter Review
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+
Attachment #595373 - Attachment is obsolete: true
Attachment #595379 - Flags: review?(jwein)
Attachment #595379 - Flags: review?(dao)
Attachment #595379 - Flags: review?(dao) → review+
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)
Attachment #595379 - Flags: review?(jwein)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: