Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update full screen API. #358

Merged
merged 6 commits into from
Mar 27, 2014

Conversation

avandecreme
Copy link
Member

While fixing #347 I ended up updating the whole API as the methods names where not up to date.

I am wondering if we can remove the MIT license. The only code which is potentially under MIT is the definition of the fullScreenApi object. All other code has been rewritten.

@avandecreme
Copy link
Member Author

I will need testers. Here is a test url: http://avandecreme.github.io/openseadragon-debug-site/

Things to test:

  • Can you type in the input text while in fullscreen
  • OpenSeadragon still working.

I already tested:

  • Opera Linux (implement the standard API)
  • Chromium Linux (webkit prefix)
  • Firefox Linux (moz prefix + FullScreen instead of Fullscreen)
  • IE 11 (ms prefix + events camel case)

Tests needed:

  • IE < 11 (no support but activeX)
  • Safari (webkit prefix but will probably complain about Element.ALLOW_KEYBOARD_INPUT)

@avandecreme
Copy link
Member Author

Got access to IE11 and fixed a bug.
The doc here http://msdn.microsoft.com/en-us/library/ie/dn265028%28v=vs.85%29.aspx is telling that msFullscreenElement returns undefined when not in full screen but it actually returns null.

@msalsbery
Copy link
Member

On Windows 7...

IE 11
Text box ok, no viewer after clicking "Toggle Fullscreen"

FF 27.0.1
Text box ok, no viewer after clicking "Toggle Fullscreen"

Chrome 33.0.1750.154 m
Text box ok, no viewer after clicking "Toggle Fullscreen"

Opera 20.0.1387.77
Text box ok, no viewer after clicking "Toggle Fullscreen"

@avandecreme
Copy link
Member Author

Actually I have 2 tests here:

  • toggle fullscreen on the input text
  • toggle fullscreen on OpenSeadragon with the usual button

So it seems to work fine for you.
Thanks for testing.

@msalsbery
Copy link
Member

Actually I have 2 tests here

Oh ok, sorry :) Then yes works fine.

@iangilman
Copy link
Member

On Mac Mavericks, tested on Firefox, Chrome, Safari and Opera. Everything worked fine except you can't type in fullscreen in Safari.

@iangilman
Copy link
Member

Code looks good.

On the license question, I'm certainly not an expert. It does seem like more or less a complete rewrite, so it seems like we could reasonably lose the MIT license. @bgilbert might have thoughts, as well as @johndyer, who wrote the code we started with.

@avandecreme
Copy link
Member Author

On Mac Mavericks, tested on Firefox, Chrome, Safari and Opera. Everything worked fine except you can't type in fullscreen in Safari.

Good.
From what I read, Apple does not want to allow typing in fullscreen so that is the best we can do.
We still need a feedback from someone with an IE < 11.

@msalsbery
Copy link
Member

I ran it through IE11 dev tools in IE8, 9, 10 modes - all gave the error "Automation server can't create object" at the "var wscript = new ActiveXObject("WScript.Shell");" line.

I could get it to work by trusting the site and relaxing the activex security settings as described in the last answers here

That may be a problem for regular users....just thought I'd mention it in case it helps. Or, it could be a problem with just the legacy version emulation...still need test on real IE versions. :)

@avandecreme
Copy link
Member Author

Ok thanks for testing.
I am thinking to remove the activex part. OpenSeadragon would just go in fullpage.

@msalsbery
Copy link
Member

That reminds me...The F11 just did a pseudo full page where the title and menu bars etc. are removed giving the page as much real estate as possible. Not a real full screen. That too could be just a missing feature of the emulation mode, but I think that may be what you'd get on the real versions if I recall...I could be wrong.

@avandecreme
Copy link
Member Author

Ok, I removed the activeX part so that only real full screen mode is supported.

Also, it seems that Element.ALLOW_KEYBOARD_INPUT might crash old Safari. See code-lts/jquery-fullscreen-plugin#11
What versions of Safari are we supporting?
I am also surprised that they mention that Chrome doesn't need the parameter as discovering that I needed it, is what made me dig into this.

@avandecreme
Copy link
Member Author

Alright, I figured out what was going on with Chrome.
Element.ALLOW_KEYBOARD_INPUT is needed with the old method webkitRequestFullScreen but not with the new one (webkitRequestFullscreen with a lower case S).

I am wondering if we are dropping full screen support on old Safari by using the lower case S though.

@iangilman
Copy link
Member

How old of a Safari are you talking about? If you go by the browser usage stats:

http://caniuse.com/usage_table.php

...looks like there's a little bump at 5.1, but 5.0 and certainly 4.x don't seem to be getting much use.

I guess we still haven't defined an explicit browser support list, but there's some discussion in #106; perhaps it's worth revisiting.

@iangilman
Copy link
Member

If we want to support webkitRequestFullScreen, it should be easy enough to test for the existence of element.requestFullScreen, right?

The code changes look good.

In the absence of response from @bgilbert or @johndyer, I'm happy to go either way on the license. It does seem like a full rewrite, FWIW.

@avandecreme
Copy link
Member Author

If we want to support webkitRequestFullScreen, it should be easy enough to test for the existence of element.requestFullScreen, right?

Good idea. I added support for old Webkit just in case some old Safari supports it but not the new API.
And I added getFullScreenElement.

I also removed the license.

I updated the test page here: http://avandecreme.github.io/openseadragon-debug-site/

@msalsbery
Copy link
Member

I updated the test page

Tested in IE 8, 9, 10 emulation on IE 11... toggle fullscreen button has no effect (no errors reported), viewer button does full page.

@avandecreme
Copy link
Member Author

Tested in IE 8, 9, 10 emulation on IE 11... toggle fullscreen button has no effect (no errors reported), viewer button does full page.

Thanks! The comportment sounds good to me. I think it is up to the API user's to properly handle the case where the full-screen is not supported (like we do in OSD by switching to full-page instead).

@iangilman
Copy link
Member

Tested again on Mac Firefox, Chrome and Safari; looking good.

Code looks good too. Is this ready to land?

@avandecreme
Copy link
Member Author

Yes! Thanks for testing.

iangilman added a commit that referenced this pull request Mar 27, 2014
@iangilman iangilman merged commit dc3eebc into openseadragon:master Mar 27, 2014
@iangilman
Copy link
Member

Thank you for making it happen!

@avandecreme avandecreme deleted the fullscreen-inputs branch March 27, 2014 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants