Closed Bug 531576 Opened 15 years ago Closed 15 years ago

Port full screen video playback to SeaMonkey

Categories

(SeaMonkey :: UI Design, enhancement)

Product:
SeaMonkey
Component:
UI Design
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.1a1

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(1 file, 4 obsolete files)

Add full screen video v3.1 [Checkin: Comment 17]
21.66 KB, patch
bugzilla
: review+
neil
: superreview+
Details | Diff | Splinter Review
Attached patch Add full screen video (obsolete) — — Splinter Review 
Full screen video playback (bug 453063) should be ported to SeaMonkey.  Here is a patch.
Attachment #414936 - Flags: review?(neil)
Blocks: FF2SM
Comment on attachment 414936 [details] [diff] [review]
Add full screen video

>+<!DOCTYPE html>
Nit: I'd prefer the DOCTYPE after the licence, if possible.

>+<!--
>+# ***** BEGIN LICENSE BLOCK *****
Nit: XML rather than shell licence block.

>+<html xmlns="http://www.w3.org/1999/xhtml">
[I wonder why this is done in HTML instead of XUL]

>+  <style type="text/css"><![CDATA[
[Any reason that these are inline?]

>+}
>+body {
Nit: blank line between each ruleset

>+  background: black;
Nit: background-color

>+body.userIdle {
>+  cursor: none;
This didn't seem to work for some reason...
[Edit: it does work to hide the cursor later. Maybe it needs to be scripted?]

>+window.addEventListener("focus", function () {
There are a lot of anonymous functions here... any chance you could rewrite it using named (not inline) functions?

>+    video.play();
That's annoying. (Full-screening a paused YouTube video is still paused.)

>+    if (!video.paused && !video.ended) {
>+      video.pause();
>+      contentVideo.play();
But exiting a paused video works!

>+var idleTimer;
>+function resetIdleTimer() {
>+  if (idleTimer) {
[idleTimer possibly uninitialised at this point.]

>+    clearTimeout(idleTimer);
It's not an error to clear a fired or cleared timeout, so don't bother checking the value of idleTimer or zeroing it (except to init it).

>diff --git a/suite/themes/classic/communicator/icons/close.png b/suite/themes/classic/communicator/icons/close.png
[Yet another close image? Not one that I particularly like, but maybe name this closeFullScreenVideo.png to avoid confusion...]
I'm not sure whether it's because of the patch but I managed to get the original non-full-screened video into a state where I had to click the play/pause button more than once before it took effect.
Attached patch Add full screen video v2 (obsolete) — — Splinter Review 
(In reply to comment #1)
> Nit: I'd prefer the DOCTYPE after the licence, if possible.

Fixed in this updated patch.

> Nit: XML rather than shell licence block.

Fixed.

> [I wonder why this is done in HTML instead of XUL]

I could port it over to XUL, but I'm not sure how to use a <video> tag in a XUL document.

> [Any reason that these are inline?]

I moved them to the theme stylesheets.

> Nit: blank line between each ruleset

Fixed.

> Nit: background-color

Fixed.

> This didn't seem to work for some reason...
> [Edit: it does work to hide the cursor later. Maybe it needs to be scripted?]

I removed the userIdle class and moved the code to show/hide the cursor and close button into the showUI() and hideUI() functions.

> There are a lot of anonymous functions here... any chance you could rewrite it
> using named (not inline) functions?

Fixed.

> That's annoying. (Full-screening a paused YouTube video is still paused.)
> 
> But exiting a paused video works!

I added some code to check whether or not the video was paused when entering full screen mode.

> [idleTimer possibly uninitialised at this point.]

Fixed.

> It's not an error to clear a fired or cleared timeout, so don't bother checking
> the value of idleTimer or zeroing it (except to init it).

Fixed.

> [Yet another close image? Not one that I particularly like, but maybe name this
> closeFullScreenVideo.png to avoid confusion...]

Firefox reuses the close button from their ctrl-tab UI, so I had renamed it and copied it over.  I changed the name to your suggestion, though.

(In reply to comment #2)
> I'm not sure whether it's because of the patch but I managed to get the
> original non-full-screened video into a state where I had to click the
> play/pause button more than once before it took effect.

I haven't encountered this in my testing so far, but I'll look out for it.
Attachment #414936 - Attachment is obsolete: true
Attachment #416203 - Flags: review?(neil)
Attachment #414936 - Flags: review?(neil)
(In reply to comment #3)
> (In reply to comment #1)
> > [I wonder why this is done in HTML instead of XUL]
> I could port it over to XUL, but I'm not sure how to use a <video> tag in a XUL document.
You would just put it into the HTML namespace.

> (In reply to comment #2)
> > I'm not sure whether it's because of the patch but I managed to get the
> > original non-full-screened video into a state where I had to click the
> > play/pause button more than once before it took effect.
> I haven't encountered this in my testing so far, but I'll look out for it.
I hit it again by accident, but I still don't know what I did to trigger it.
Comment on attachment 416203 [details] [diff] [review]
Add full screen video v2

>+window.addEventListener("focus", onWindowFocus, false);
>+window.addEventListener("unload", onWindowUnload, false);
>+window.addEventListener("keypress", onKeyPress, false);
Ah yes, this looks much better. I'm not convinced that you need to include "Window" in the name of the function though.

>+function onWindowFocus() {
>+  window.removeEventListener("focus", arguments.callee, false);
Nit: you can now use the actual function name instead of arguments.callee

>+function onSeek() {
>+  video.removeEventListener("seeked", arguments.callee, false);
Nit: you can merge this into playbackStarts, it's not an error to remove the event listener that you didn't add.

>+  document.body.classList.remove("loadingdata");
Nit: there's only one class, so no need to meddle with a list.
I wonder whether we can set the background on the <html> and make the body hidden using inline style.

>+  clearTimeout(idleTimer);
>+
>+  idleTimer = setTimeout(initIdleTimer, 2000);
Nit: don't need the blank line

>+function initIdleTimer() {
>+  idleTimer = 0;
>+  hideUI();
Nit: you don't need to zero idleTimer, so you can call hideUI directly.

>+    document.getElementById("close").style.visibility = "visible";
I wonder why we use an id for the close button but a selector for the video.

>+    document.body.style.cursor = "none";
I still couldn't get this to work reliably. I also tried window.setCursor("none") but that wasn't reliable either. Maybe we need both.

>+  <span id="close" onclick="window.close();"/>
When you absolutely position a span it gets treated internally as if it was a div (so you can set height and width on it). Might as well use a div...

>+  top: 0;
>+  right: 0;
Nit: 0px please.

>+  background: url("chrome://communicator/skin/icons/closeFullScreenVideo.png") center center no-repeat;
I still think this image is ugly, but as yet the output of my lack of graphics design skills is even worse.
Attached patch Add full screen video v3 (obsolete) — — Splinter Review 
(In reply to comment #4)
> You would just put it into the HTML namespace.

I tried, but so far, I haven't been able to get the video to display or the window to take up the whole screen.

(In reply to comment #5)
> Ah yes, this looks much better. I'm not convinced that you need to include
> "Window" in the name of the function though.

Fixed in this new patch.

> Nit: you can now use the actual function name instead of arguments.callee

Fixed.

> Nit: you can merge this into playbackStarts, it's not an error to remove the
> event listener that you didn't add.

Fixed.

> Nit: there's only one class, so no need to meddle with a list.
> I wonder whether we can set the background on the <html> and make the body
> hidden using inline style.

I switched to what you suggested, and it works just as well.

> Nit: don't need the blank line

Fixed.

> Nit: you don't need to zero idleTimer, so you can call hideUI directly.

Fixed.

> I wonder why we use an id for the close button but a selector for the video.

Oops, I'm just used to using ids.  I switched to a selector for the close button.

> I still couldn't get this to work reliably. I also tried
> window.setCursor("none") but that wasn't reliable either. Maybe we need both.

I see what you mean.  For me, window.setCursor("none") does hide the cursor every time (as opposed to document.body.style.cursor, which didn't work in some cases), so I switched to that.  There are still times when the cursor reappears for no apparent reason, though, and I'm not sure what is causing that.  Unfortunately, using both doesn't help.

> When you absolutely position a span it gets treated internally as if it was a
> div (so you can set height and width on it). Might as well use a div...

Fixed.

> Nit: 0px please.

Fixed.

> I still think this image is ugly, but as yet the output of my lack of graphics
> design skills is even worse.

I can make an attempt at a new icon, although I can't guarantee that it will look better (or even as good) as what's there now.  If I come up with anything, I'll attach it separately and request ui-review since I don't want to hold up the review of the rest of the patch with that.
Attachment #416203 - Attachment is obsolete: true
Attachment #416664 - Flags: review?(neil)
Attachment #416203 - Flags: review?(neil)
(In reply to comment #6)
> > You would just put it into the HTML namespace.
> I tried, but so far, I haven't been able to get the video to display or the
> window to take up the whole screen.
Don't waste time on it, but I had a couple of ideas such as putting it into a <stack> element (which would help to position the close button in XUL).


> > I wonder why we use an id for the close button but a selector for the video.
> Oops, I'm just used to using ids.  I switched to a selector for the close
> button.
Sorry I wasn't clear, but that's not what I meant, since your selector is still using an id; I was able to think up with three choices, which are a) add an id to the video and use getElementById throughout b) remove the id from the close button and use a div selector c) remove the id from the close button and use document.body.first/lastElementChild.

> > I still couldn't get this to work reliably. I also tried
> > window.setCursor("none") but that wasn't reliable either. Maybe we need both.
> I see what you mean.  For me, window.setCursor("none") does hide the cursor
> every time (as opposed to document.body.style.cursor, which didn't work in some
> cases), so I switched to that.  There are still times when the cursor reappears
> for no apparent reason, though, and I'm not sure what is causing that. 
> Unfortunately, using both doesn't help.
I only saw the cursor reappear in one case, and I seem to recall that using both helped there, but maybe I should try several machines to make sure.

> I can make an attempt at a new icon, although I can't guarantee that it will
> look better (or even as good) as what's there now.  If I come up with anything,
> I'll attach it separately and request ui-review since I don't want to hold up
> the review of the rest of the patch with that.
I was going to have another go at it myself today, but probably after I review.
Comment on attachment 416664 [details] [diff] [review]
Add full screen video v3

r=me once the id inconsistency is sorted one way or another.
Attachment #416664 - Flags: review?(neil) → review+
(In reply to comment #7)
> Sorry I wasn't clear, but that's not what I meant, since your selector is still
> using an id; I was able to think up with three choices, which are a) add an id
> to the video and use getElementById throughout b) remove the id from the close
> button and use a div selector c) remove the id from the close button and use
> document.body.first/lastElementChild.

Oh, I see.  I updated the patch to fix that.  Carrying over your r+ and requesting sr.

> I only saw the cursor reappear in one case, and I seem to recall that using
> both helped there, but maybe I should try several machines to make sure.

I did some more testing on this, and I can only get the cursor to reappear if the video is still downloading and if I use the "f" access key to choose "Full Screen" rather than clicking the menu entry.  Even then, I still can't get it to happen every time.  I've only tested using my Windows XP-based dev box, by the way, so I'm not sure if this occurs on other platforms.

> I was going to have another go at it myself today, but probably after I review.

Sounds good.  I haven't come up with any good ideas for the button so far, so I'll hold off on attempting to create one.
Attachment #416664 - Attachment is obsolete: true
Attachment #416845 - Flags: superreview?(neil)
Attachment #416845 - Flags: review+
(In reply to comment #9)
> (In reply to comment #7)
> > I only saw the cursor reappear in one case, and I seem to recall that using
> > both helped there, but maybe I should try several machines to make sure.
> I did some more testing on this, and I can only get the cursor to reappear if
> the video is still downloading and if I use the "f" access key to choose "Full
> Screen" rather than clicking the menu entry.  Even then, I still can't get it
> to happen every time.  I've only tested using my Windows XP-based dev box, by
> the way, so I'm not sure if this occurs on other platforms.
I think it must be something to do with changing the visibility of the body. That's why I thought we needed both setCursor and the cursor style. (I can't obviously see how you would call setCursor again from playbackStarts since you don't know at that point whether the cursor should be visible or not.)
Comment on attachment 416845 [details] [diff] [review]
Add full screen video v3.1
[Checkin: Comment 17]

Let's go with this for now and we can take another look at it if we get complaints about the cursor not hiding properly.
Attachment #416845 - Flags: superreview?(neil) → superreview+
Keywords: checkin-needed
Depends on: 453063
Whiteboard: [c-n: that's an utf-8 patch...]
Target Milestone: --- → seamonkey2.1a1
(In reply to comment #5)
> I wonder why we use an id for the close button but a selector for the video.

"div" is ambiguous, "#close" and "video" aren't.
Attached patch Patch for checkin (obsolete) — — Splinter Review 
Here is the same patch as v3.1 with ANSI encoding.
Comment on attachment 420278 [details] [diff] [review]
Patch for checkin

>diff --git a/suite/common/fullscreen-video.xhtml b/suite/common/fullscreen-video.xhtml
>@@ -0,0 +1,187 @@

ANSI patch containing UTF-8 encoding:

>+<?xml version="1.0" encoding="UTF-8"?>

Not containing any "extended" char then!?

>+   -   Dão Gottwald <dao@mozilla.com>

Except this one!?
(In reply to comment #14)
> Not containing any "extended" char then!?

> Except this one!?

Yes, that's the only one in the patch.
Comment on attachment 420278 [details] [diff] [review]
Patch for checkin


qimportbz reports
"UnicodeDecodeError: 'utf8' codec can't decode bytes in position 2391-2393: invalid data".
Not sure if the patch or the program is at fault,
I'll use the utf8 patch then.
Attachment #420278 - Attachment is obsolete: true
Depends on: 533890
Comment on attachment 416845 [details] [diff] [review]
Add full screen video v3.1
[Checkin: Comment 17]


http://hg.mozilla.org/comm-central/rev/f822966e6453
with s/Dão/Dao/ :-|
Attachment #416845 - Attachment description: Add full screen video v3.1 → Add full screen video v3.1 [Checkin: Comment 17]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Component: General → UI Design
Keywords: checkin-needed
QA Contact: general → ui-design
Resolution: --- → FIXED
Whiteboard: [c-n: that's an utf-8 patch...]
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: