apache / cordova-docs

Apache Cordova Documentation
https://cordova.apache.org/
Apache License 2.0
350 stars 554 forks source link

CB-8783 Document iOS specific callback for orientaion #276

Closed nikhilkh closed 9 years ago

purplecabbage commented 9 years ago

Not entirely correct. If you want to programmatically decide at runtime whether or not support an orientation, you need to a) define support for multiple orientations () b) provide the window.shouldRotateToOrientation method and return true if you want to rotate to the new orientation.

nikhilkh commented 9 years ago

This callback is required even if you do not want programmatic control. By default, the webview will not cause a rotation as the iOS code expects a callback to be defined and if none is defined - it will not support both landscape and portrait orientations. https://github.com/apache/cordova-ios/blob/ed54ddf2cc6e2b746a406e15e99f549846cd171b/CordovaLib/Classes/CDVViewController.m#L555

purplecabbage commented 9 years ago

The fallback if you do not provide shouldRotateToOrientation is to evaluate based on what is specified in the plist. The plist values come from config.xml preferences. This covers all cases. https://issues.apache.org/jira/browse/CB-6462

nikhilkh commented 9 years ago

config.xml does not support plist values to be set to landscape & portrait: https://github.com/apache/cordova-lib/blob/8de97feba2fa883298eb9c99be0c425c73960f1a/cordova-lib/src/cordova/metadata/ios_parser.js#L84

'default' allows both portrait & landscape mode - only after implementing the callback. I could perhaps re-word this as follows:

For iOS, orientation can be programmatically controlled by defining a javascript callback on window:

    /** 
    * @param {Number} degree - UIInterfaceOrientationPortrait: 0, UIInterfaceOrientationLandscapeRight: 90, UIInterfaceOrientationLandscapeLeft: -90, UIInterfaceOrientationPortraitUpsideDown: 180
    * @returns {Boolean} Indicating if rotation should be allowed.
    */
    function shouldRotateToOrientation(degrees) {
         return true;
    }
purplecabbage commented 9 years ago

Actually, that is a bug. If 'default' is specified in config.xml, it should produce plist values to support both landscape and portrait. Please file an issue. That wording is definitely better :)

nikhilkh commented 9 years ago

Thanks, @purplecabbage, Made the update to the docs commit and created a JIRA to track the bug: https://issues.apache.org/jira/browse/CB-8783.

mmrko commented 9 years ago

Actually, that is a bug. If 'default' is specified in config.xml, it should produce plist values to support both landscape and portrait.

I think this is more about the semantics of default. IMO the default orientation translates to: let the platform decide how to handle the orientation, that is, fall back to whatever is the platform's default behaviour by removing the corresponding orientation and supported orientation entries from the platform's manifest.

This approach of course has, as @nikhilkh pointed out, the downside that one can not explicitly set the orientation to support both portrait and landscape via config.xml.

Thus, to add flexibility and to address the inherent confusion around orientation default I'd suggest adding support for supported-orientations preference to config.xml — something that apache/cordova-lib#128 would've also benefited from. This would allow to work around the aforementioned iOS issue like so:

<preference name="orientation" value="default" />

<platform name="ios">
    <preference name="supported-orientations" value="portrait, landscape" />
</platform>

If this is something you feel would make sense I'd be happy to create a PR for the changes over at apache/cordova-lib.

Ping @agrieve

PS. Hope I didn't come across as an ass. Got nothing but respect for people like @purplecabbage for all the effort put in this amazing project :)

shazron commented 9 years ago

That JS function shouldRotateToOrientation will not be supported in cordova-ios 4.0.x. This is because of the pluggable webview support. The CDVWebViewEngine interface calls for an async call to JavaScript now (WKWebView's function to call into JS is async).

Because this call is async, the supportedOrientations selector that is called by the CDVViewController synchronously, can't use the async function above.

shazron commented 9 years ago

I'm not in favour of adding yet another preference.

If we look at the Apple docs: https://developer.apple.com/library/ios/documentation/General/Reference/InfoPlistKeyReference/Articles/iPhoneOSKeys.html#//apple_ref/doc/uid/TP40009252-SW10

Setting no values, implicitly sets the orientation to Portrait. Thus "default" === "portrait" here. I'm not sure this is really useful for users, especially if they don't know what "default" is (but that's a matter of documentation). Of course this varies by platform.

I would leave default as implemented by @mmrko -- since it is technically correct -- but add a new preference value "all" which is unambiguously clear.

shazron commented 9 years ago

@nikhilkh can you edit the title to have a prefix of CB-8783 to enable auto JIRA tracking.

msct commented 9 years ago

I'll offer my two cents. Yes, ambiguous and technically correct, and that's why default should have probably always been the device default. But these are the semantics which describe the intent and goal of Cordova as a whole....

I don't use Cordova for device defaults. We use Cordova for a familiar development pattern and a consistent experience across all devices. Having a root-level orientation preference which allows for unrestricted orientation accomplishes that.

As @shazron suggested, I am very much in favor of an all, or any value. It's one thing to change an already well-documented config.xml naming peculiarity, but it's another thing to remove it outright and force code changes. Lots of love guys.

shazron commented 9 years ago

By silent consensus -- I'll just add the "all" option for iOS and document it.

ghost commented 8 years ago

@shazron Looks like you reverted the "all" option in a05f448b48dd7634696fcf2ece91e0b978348be9 Was hoping to see this land in Cordova 5.1.1. Are there plans to re-introduce this change?

shazron commented 8 years ago

No, it only has been reverted for all platforms. You can add it under the iOS platform and it should work. It is already released in the latest cordova-cli, see: http://cordova.apache.org/docs/en/5.1.1/config_ref_index.md.html#The%20config.xml%20File

ghost commented 8 years ago

oops, my bad. Thanks for the clarification