immersive-web / webvr-polyfill

Use WebVR today, without requiring a special browser build.
http://immersive-web.github.io/webvr-polyfill/examples/
Apache License 2.0
1.4k stars 323 forks source link

Exiting VR does not remove full screen wrapper on iOS #108

Closed brianchirls closed 7 years ago

brianchirls commented 8 years ago

The full screen wrapper is removed inside the fullscreenchange event listener, which never runs on iOS because there's no native full screen support. We need to force that function to run after calling exitPresent.

As a result, the full screen wrapper stays in place and things generally go wrong.

cvan commented 8 years ago

/cc @dmarcos

brianchirls commented 8 years ago

It's also worth noting that iOS does this thing where if you touch the top or bottom edge of the screen (while in landscape and with the URL bar hidden), it will reveal the URL bar, therefore totally ruining any kind of VR experience. If we can figure out how to listen for that event, it might be a good time to trigger both fullscreenchange and exitPresent, in whichever order seems appropriate.

cvan commented 8 years ago

yeah, I hate that. last time I checked, I couldn't find a reliable way to disable that.

confirming you have these <meta> tags?

<meta name="viewport" content="width=device-width,initial-scale=1,maximum-scale=1,shrink-to-fit=no,user-scalable=no,minimal-ui">
<meta name="apple-mobile-web-app-capable" content="yes">
brianchirls commented 8 years ago

Yup, I do, except for minimal-ui, 'cause it was disabled in iOS 8.

It also makes the cardboard button more or less useless, since that's right in the spot where the lever trigger thingy hits the screen.

brianchirls commented 8 years ago

I've attempted a fix for this, and it has solved the problem of removing the full screen wrapper, but it has left unsolved a number of other problems:

  1. Upon touching the back arrow, the full screen wrapper is restored, the problem from #102 and #106 returns, drawing the viewport at the wrong size. This only happens about half the time.
  2. Touching the top/bottom edge of the screen displays the URL bar but does not exit VR mode. And the viewport is too big. I suspect that this could be solved by listening for a resize event and forcing exit if the screen dimensions indicate that it's making space for a URL bar.
  3. On iPhone 5, if you start in portrait mode, touch the VR button and then rotate to landscape, the URL bar and button bar do not go away. I'm not seeing this on iPhone 6 in the simulator, and I don't have a real iPhone 6 here at the moment. I may tomorrow and will test if I have time.

The full screen wrapper was a valiant effort, but I wonder if it's causing more trouble than it's worth and we should pursue another architecture. None of the above problems existed before the wrapper was introduced with WebVR 1.0 support. Many of us have invested an enormous amount of time debugging and attempting to fix problems with the iPhone and this wrapper, and it's still not working.

Your thoughts please: @cvan, @dmarcos, @borismus, @toji. Thanks.

cvan commented 8 years ago

The full screen wrapper was a valiant effort, but I wonder if it's causing more trouble than it's worth and we should pursue another architecture. None of the above problems existed before the wrapper was introduced with WebVR 1.0 support. Many of us have invested an enormous amount of time debugging and attempting to fix problems with the iPhone and this wrapper, and it's still not working.

IMO we shouldn't be using Fullscreen on mobile if the WebVR v1.0 isn't present. Fullscreen-ing the canvas doesn't introduce many benefits that I can think of - and in fact causes quite a few inconsistencies between "entering VR" and "exiting VR" on Android vs. iOS (and on whichever other platforms the Fullscreen API is unimplemented):

Beyond all the caveats you've mentioned above, with Chrome for Android, you must fullscreen the canvas not the body, which leads to having to render UI icons (like the back button, divider line, and the settings gear icon) in raw WebGL. And at least with Firefox + Chrome for Android, we can still do "wakelock" and orientation lock without needing to use the Fullscreen API.

I'm very pleased with all the hard work everyone's done to get the webvr-polyfill updated with WebVR v1.0 support. It has, at least in my experience, become very complicated now to maintain, track down regressions, and provide a first-class experience for anything but Chrome for Android. I understand that's the focus, but things were working decently before. The changes to the API IMO aren't the problem. I think it's more that the webvr-polyfill has consumed the webvr-boilerplate, and there are a lot of paper cuts unaddressed in a library that I think folks expect to be stable. Early times, fail fast, I know - but what can we do today to simplify this?

I know the awesome folks at eleVR are working on a new boilerplate that's coming soon. Perhaps this webvr-polyfill should become what the webvr-boilerplate is/was, and we/someone/eleVR should abstract out the necessary agnostic bits (IMO, anything that is specific to the spec and not specific to Cardboard, distortion, etc.) into a separate.

brianchirls commented 8 years ago

Big 👍 to everything in your third paragraph.

Your suggested path, however, goes a bit farther than I was thinking. As of now, Cardboard is the only platform for which a polyfill is even conceivable, so I don't think it's necessary yet to pull those pieces out of this repo. And the files are organized in a way that I don't think it'd be too hard to separate the Cardboard-specific components into a plugin in another repo if that ever changed. The distortion and sensor fusion are extremely helpful, and @borismus did a really good job of hiding distortion inside submitFrame.

I don't think we need to throw the baby out with the bathwater, and as brilliant as eleVR are it's impractical to wait around for them to finish up code for which they haven't yet shown any progress or given an estimated timeline. However, I'm not opposed to finding places where the existing code could be more modular. It might be nice to be able to customize how some things work, like the look of the Cardboard UI components, or to cut out some code that I'm not using.

In my experience, full screen canvas hasn't been that big of a problem, when it works. Probably the biggest annoyance has been dealing with the extra "fullscreenchanged" events, but I think we've managed to work around that one. It was working before the API change, and I think it's a helpful layer protection (on Android) against some weird UI states.

That said, we obviously can't rely on full screen since iPhone doesn't support it. If we can come up with a simpler and more reliable solution there and full screen canvas doesn't offer any benefits on top of that solution, then I'm happy to remove it.

It seems to me the problems started showing up around 05e9d7d1d30a67c7173d1ab15d803aaf613d8fa9, probably due to iOS's inability to correctly handle moving objects around inside the DOM. I'm happy to attempt another solution, but I'd like some input from @toji on it since it's his code. According to his lengthy commit message (which I appreciate, BTW), this was an attempt to keep non-canvas buttons from interfering with the canvas, but since we've since been forced to use z-index: 999999 !important on the wrapper anyway, maybe we should just throw that on the canvas and not bother with the wrapper.

Does anybody know any reason that wouldn't work?

brianchirls commented 8 years ago

Upon further thought, there is at least one scenario in which full screen is actively helpful: displaying VR from inside an iframe. If the iframe has the appropriate attribute(s), you can show the canvas at full screen. On iOS, you have to pop the iframe out into its own tab/window, which is an okay fallback but sucks as an experience. So it'd be nice to not have to do that on Android.

...though I suppose I might let you convince me that this should be handled by the boilerplate.

borismus commented 8 years ago

The fullscreen wrapper was introduced mainly so that other projects could provide an extra UI layer that would be visible in VR mode.

It's mainly done for the WebVR boilerplate's buttons, which help you transition between normal, full-screen and VR mode. I'm open to removing the wrapper for simplicity's sake.

My preference is to build UI using HTML/CSS, which makes rendering easy, lets you handle hover states, proper target sizing. What we have now in the polyfill is an ugly hack: we are forced to render the UI with WebGL, and set up our own target size handling. This is overly complex and brittle, and a shame, given the fact that we have the web platform at our disposal.

But I don't know how to mix WebGL and HTML/CSS without having a wrapper element. Are there any alternatives that come to mind? Could we have some hooks in the polyfill that would allow developers to render their own UIs in Canvas?

brianchirls commented 8 years ago

Thanks for your thoughts @borismus and @cvan.

I'm a bit torn on allowing customization of the UI. On the one hand, yes, customization = good. On the other hand, extra DOM tends to get in the way and complicate things, and we're not going to be able to customize the browser's UI once the webvr is handling all this stuff for us anyway.

I've been hacking away at this today, and it will probably be my top/only priority until it gets resolved. Here's what I've discovered:

  1. The wrapper as a cause of the iPhone sizing problems is speculation on my part, and I may be way off here.
  2. The size problem shows up at weird times, but always seems to be fixed by rotating the phone.
  3. We never had this problem before the WebVR 1.0 refactor, so we know a solution is possible. But it is very difficult to nail down a single cause, since there are so many components that changed all at once: polyfill, boilerplate, VREffect. And there were some big commits with dependencies that make it more complex.
  4. Disabling distortion doesn't make the problem go away.

I can think of a few things we can change about process and structure to try and avoid situations like this in the future, and maybe we should discuss that at some point. But right now, we've got to fix this. I will keep hacking away at it, but I'm still a bit lost. At this point the only reliable answers I can think of are to either start deleting bits of code until the problem goes away or to rebuild the whole mess from scratch. I don't want to do either. Open to suggestions. :-(

brianchirls commented 8 years ago

Okay, I feel stupid. I was working off some old code. Everything is fine. Apologies to all around, especially @toji - sir, your code is perfect and beautiful. Please feel free to keep discussing complexity and stuff, 'cause that's all valid.

I've updated my branch and it works. The only thing it does not address is what happens when touching the edge of the screen makes the browser chrome pop up. I suggest exiting VR mode to keep things simple and straightforward...well as much as possible on iPhone.

I'll start working on that and do some more tests before making a PR.

toji commented 8 years ago

You, sir, are slightly delusional but my ego thanks you regardless. :)

I don't dispute that the current code is fairly complex, but I will argue that there's good reason for using a proper fullscreen when it's available. It clears out the navigation bars and ensures that the distortion meshes line up correctly, and allows the use of orientation lock. Taken together I find those improvements worthwhile. The issues with the wrapper element are unfortunate, though. Maybe I can migrate more of the UI to raw WebGL? (Not great for maintainability, but if it helps stability I'm all for it.)

borismus commented 8 years ago

To be honest, there's not much more UI to migrate. Boilerplate just adds the mode changing buttons. We don't need anything else.

Is there a way to give developers a handle on the 3D canvas and let them implement those customizations themselves? Perhaps we could let developers specify image URL, position on screen and callback. Actually this seems like a generally useful as a way of building an on-screen HUD. Might be useful in the general WebVR case.

On Thu, Jun 2, 2016 at 12:59 PM Brandon Jones notifications@github.com wrote:

You, sir, are slightly delusional but my ego thanks you regardless. :)

I don't dispute that the current code is fairly complex, but I will argue that there's good reason for using a proper fullscreen when it's available. It clears out the navigation bars and ensures that the distortion meshes line up correctly, and allows the use of orientation lock. Taken together I find those improvements worthwhile. The issues with the wrapper element are unfortunate, though. Maybe I can migrate more of the UI to raw WebGL? (Not great for maintainability, but if it helps stability I'm all for it.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/borismus/webvr-polyfill/issues/108#issuecomment-223404486, or mute the thread https://github.com/notifications/unsubscribe/AAQ8gOO_3clw8g_PE6RKwyN4jPNEavMEks5qHzY1gaJpZM4IcPbk .

toji commented 8 years ago

The UI I was thinking of migrating was the orientation splash screens. They're the only reason for having the wrapper element that I'm aware of at this point.

brianchirls commented 8 years ago

Okay, some more progress here. I've now got it exiting VR when you tap the edge to reveal the URL bar. Two problems remain:

  1. On an iPhone 5, if you enter VR when the device is in portrait orientation and then rotate it to landscape, it immediately exits "full screen" revealing the URL bar on top and buttons on the bottom. This does not happen without the wrapper. Does anyone know what Mobile Safari's rules are for triggering this behavior?
  2. The polyfill will support presentation as Cardboard on an iPad. This is probably not something we want to support, since it's not really practical. Not a deal-breaker for me, since my application code already blocks this.
borismus commented 8 years ago

Given that there's no actual fullscreen support on iOS, I'm wondering if we should even bother with the fullscreen wrapper. This would simplify things.

borismus commented 8 years ago

@brianchirls Have you managed to reliably detect the gesture of tapping the edge to reveal the URL bar? Also, in my experience I tap the edge quite often. If every time that happens you leave VR mode, I can imagine that being quite frustrating.

In the meantime, I've committed a few tweaks to disable the fullscreen wrapping on iOS.

brianchirls commented 8 years ago

Yes, I've managed to detect it, but there is no way to block it. Exiting VR mode is annoying, but exposing the URL bar without a way to recover is worse. So here's what I propose. Let's get this reviewed and merged as it is so it's at least in a functioning state that makes sense, even if it's not ideal. Then we can discuss other options for recovery.

I will take a look at your tweaks and if everything is in order, I'll make a pull request this morning.

brianchirls commented 8 years ago

@borismus I'm not sure what you did here, but it seems that now the exit/back button works in iOS. I'm going to move my "touch" fix into another branch and make a PR for that.

In the future, if you don't mind, please write more descriptive commit messages than "More iOS adventures." As we discussed above, this code is all very complex and it's hard to keep track of what each change is doing. Better documentation would be helpful. Thanks.

borismus commented 8 years ago

Certainly, my apologies. The basic change is this: since we don't have fullscreen on iOS anyway, we don't do any crazy wrapping. The bulk of the fix is here: https://github.com/borismus/webvr-polyfill/blob/85f657cd502ec9417bf26b87c3cb2afa6a70e079/src/util.js#L192

Basically, iOS has a weird bug where if you change canvas.width, but not canvas.style.width (WLOG height), you end up with weird rendering sizes. So the crazy workaround is to reset the style dimensions on a timer here...

On Mon, Jun 6, 2016 at 8:42 AM Brian Chirls notifications@github.com wrote:

@borismus https://github.com/borismus I'm not sure what you did here, but it seems that now the exit/back button works in iOS. I'm going to move my "touch" fix into another branch and make a PR for that.

In the future, if you don't mind, please write more descriptive commit messages than "More iOS adventures." As we discussed above, this code is all very complex and it's hard to keep track of what each change is doing. Better documentation would be helpful. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/borismus/webvr-polyfill/issues/108#issuecomment-223998532, or mute the thread https://github.com/notifications/unsubscribe/AAQ8gGtJNESxoiBGvSvijbKmbOrgq_MGks5qJD_EgaJpZM4IcPbk .

brianchirls commented 8 years ago

Thanks for clarifying. What did you change that got the exit button working?

I just tested this out on a real iPhone 6, and it's not so bad if you don't exit VR when the URL bar shows up, 'cause the phone is huge. So I'll hold off on that PR for now. But I think it will be a problem on iPhone 5. But iPhone 5 doesn't work at all, so I'll file another issue for that...