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)
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)
8.72 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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
Comment 2•13 years ago
|
||
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•13 years ago
|
Component: DOM: Core & HTML → General
Product: Core → Firefox
QA Contact: general → general
Reporter | ||
Comment 3•13 years ago
|
||
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!
Comment 5•13 years ago
|
||
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•13 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
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•13 years ago
|
||
(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!
Assignee | ||
Comment 10•13 years ago
|
||
Also could you explain step 1! How do I go to about:config?
Comment 11•13 years ago
|
||
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•13 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!
Comment 13•13 years ago
|
||
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•13 years ago
|
||
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•13 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!
Comment 16•13 years ago
|
||
'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•13 years ago
|
||
Oh sorry, you're right. I forgot about bug 691947 which relaxed the security constraints if the request came from chrome.
Assignee | ||
Comment 18•13 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?
Comment 19•13 years ago
|
||
(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•13 years ago
|
||
Attachment #595373 -
Flags: review?(jwein)
Attachment #595373 -
Flags: review?(dao)
Comment 21•13 years ago
|
||
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•13 years ago
|
||
Attachment #595373 -
Attachment is obsolete: true
Attachment #595379 -
Flags: review?(jwein)
Attachment #595379 -
Flags: review?(dao)
Comment 23•13 years ago
|
||
Attachment #595379 -
Flags: review?(dao) → review+
Updated•13 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)
Comment 24•13 years ago
|
||
Keywords: checkin-needed
Target Milestone: --- → Firefox 13
Updated•13 years ago
|
Attachment #595379 -
Flags: review?(jwein)
Comment 25•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c047f38d9381
Thank you for the patch! :-)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 26•13 years ago
|
||
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•13 years ago
|
||
(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.
Description
•