776600 - Implement the "orientation" property of the app manifest for web apps on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Jason Smith [:jsmith]]

Implement the orientation property for web apps on Android. See https://developer.mozilla.org/en/Apps/Manifest for a description of what the orientation property does. 

Note - Current docs (as of the time of this bug filing) are slightly off - we interpret these properties:

- portrait-primary
- landscape-primary
- portrait-secondary
- landscape-secondary

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that the value is a comma-separated list.

We should support the following:

- portrait-primary

Locked to a single portrait direction. If the device has an obvious primary orientation in portrait mode, this is that orientation. If there is no obvious primary orientation due to landscape being the primary orientation for the device, this is the orientation if rotating the device 90 degrees clock-wise from the primary landscape mode.

- landscape-primary

Locked to a single landscape direction. If the device has an obvious primary orientation in landscape mode, this is that orientation. If there is no obvious landscape orientation due to portrait being the primary orientation for the device, this is the orientation if rotating the device 90 degrees clock-wise from the primary portrait mode.

- portrait-secondary

The portrait mode opposite of "portrait-primary".

- landscape-secondary

The landscape mode opposite of "landscape-primary".

- portrait

Equivalent to "portrait-primary,portrait-secondary".

- landscape

Equivalent to "landscape-primary,landscape-secondary".

Comment 2

[Wesley Johnston (:wesj)]

I think I am going to set these as a "default" state. If the app changes them using DOM Api's, I'll honor the new settings. If they then unlock using DOM API's, we'll switch back to the manifest values? Flash fullscreen also locks the orientation, but I figure we can do the same thing? Honor Flash. Revert back when it unlocks? Sound ok?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That sounds awesome!

Comment 4

[Jason Smith [:jsmith]]

Jonas - Do you know of a sample app that works on Firefox OS using the screen orientation manifest properties?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Unfortunately not.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

We have to rewrite our orientation implementation on mobile for this to work. The current spec allows you to specify something like primary_portrait,secondary_landscape and allow only those two orientations. AFAICT, Android doesn't allow that, so I'm trying to listen to rotation changes here, figure out what orientation they correspond to, and adjust.

Android doesn't seem to provide much info on what orientation a particular rotation value corresponds to either. I'm trying to use getRotation along with getConfiguration().orientation to determine the default state of the device. Seems to work, kinda sorta sometimes on the devices I have here, but I think I probably need something smarter.... or maybe we just say "Android won't let us do that" for now. Feedback/ideas blassey?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm totally fine with not having perfect support on Android for now. The only real-world app behavior that I've seen that would require a comma separated list is apps that want to allow all orientations except upside-down, so something like "primary-portrait,landscape". I think it would be ok behavior to simply ignore the orientation manifest property if we run into something like that for now.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Path 1/3

This allows setting the default orientation, and creates a pref, read at startup, to set it.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3

This gives us the ability to write a defaultprefs.js file on install of the app. Those prefs are set on first run. This relies on the patch in bug 769821.

App manifests don't actually have support for orientation yet. I'm assuming here that we'll just get back a string from them...? That's simple and will be part 3 of this patch. But maybe we'd rather do the parsing in Gecko?

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

Grr. Forgot to qref.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Webapps.jsm changes

This is the simplest way to add orientation support (I think?)

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 647709 [details] [diff] [review]
Patch 3/3 - Webapps.jsm changes

Looks good to me. Simple change, I don't think it needs extra review from someone from dom

Comment 13

[Wesley Johnston (:wesj)]

blassey, I want to point out that I didn't fix our orientation implementation here. I can file a follow up for that, but didn't want it to hold up webapps stuff.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 647707 [details] [diff] [review]
Patch

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js
>+          defaultPrefs = WebappsUI.readDefaultPrefs(FileUtils.getFile("ProfD", [WebappsUI.DEFAULT_PREFS_FILENAME]));

Move "readDefaultPrefs" to WebAppRT since it's not used in WebappsUI
Add a second const DEFAULT_PREFS_FILENAME in WebAppRT (I don't like to share)

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  DEFAULT_PREFS_FILENAME: "defaultprefs.js",

default-prefs.js"

>   doInstall: function doInstall(aData) {

>+        (function(icon) {
>           var profilePath = sendMessageToJava({

var -> let

>           var file = null;

var -> let

>           if (profilePath) {
>             var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);

"var" saved you here, but remove the "var" and don't redefine "file"

>+  writeDefaultPrefs: function webapps_writeDefaultPrefs(aFile, aPrefs) {

>+      var ostream = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream);

var -> let

>+  readDefaultPrefs: function webapps_readDefaultPrefs(aFile) {

>+    var prefsString = NetUtil.readInputStreamToString(fstream, fstream.available(), {});

var -> let

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27de96f5f67a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd4e5c2fe0a

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 647707 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New Webapps stuff
User impact if declined: Orientation isn't used, but we're also interested in using this functionality to send other information from Fennec to the apps (for instance, about whether to telemetry ping).
Testing completed (on m-c, etc.): Landed on inbound 8/8
Risk to taking this patch (and alternatives if risky): Low risk. Webapp only
String or UUID changes made by this patch: None.

Comment 17

[Jason Smith [:jsmith]]

Flagging qawanted for testing the current changes landing.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6dd4e5c2fe0a
https://hg.mozilla.org/mozilla-central/rev/27de96f5f67a

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

This specifically doesn't effect fennec

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 647709 [details] [diff] [review]
Patch 3/3 - Webapps.jsm changes

If this is a blocking feature this stuff should move forward. If its not then its probably better to just hold it. But the increased testing on Aurora would be good for us. (note there's still more work to finish this feature)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New webapps feature
User impact if declined: webapps asking for manifest.orientation will fail
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): fairly low. worst case it should return "".
String or UUID changes made by this patch: None

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 647707 [details] [diff] [review]
Patch

only affects webapps, approving for Aurora.

Comment 22

[Wesley Johnston (:wesj)]

blassey ping? I know you said you had questions about one of my patches. Can you drop them in here?

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 647700 [details] [diff] [review]
Path 1/3

Review of attachment 647700 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoScreenOrientationListener.java
@@ +119,5 @@
>    }
>  
> +  public void handleMessage(String event, JSONObject message) {
> +      try {
> +          if (event.equals("Preferences:Data")) {

as discussed, this stupid. Let's fix it in a follow up.

@@ +153,5 @@
> +      return orientationFromString(orientations.get(0));
> +  }
> +
> +  private short orientationFromString(String val) {
> +    if (val.equals("portrait"))

"portrait".equals(val) 

same for the rest

Comment 24

[Jason Smith [:jsmith]]

Note - I tried testing portrait as an app manifest property, but that did not work at all.

Comment 25

[Wesley Johnston (:wesj)]

This has not all landed yet.

Comment 26

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0255e9c7f468

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0255e9c7f468

Comment 28

[Jason Smith [:jsmith]]

FYI to Aaron for testing:

I recently did testing on this feature for Firefox OS in a session-based testing style. You can see the results of the testing I did here - https://wiki.mozilla.org/B2G/QA/Sessions/Screen_Orientation_App_Manifest_Property_%2808/21/2012%29. In short, the easiest way to test this is using testmanifest.com and creating an orientation property in the manifest with different orientation combinations.

Comment 29

[Aaron Train [:aaronmt]]

This doesn't seem to be working at all for me with the supplied property values in the manifest.

`{ "orientation" : "landscape" }` doesn't seem to work on my Galaxy Nexus — when I launch the application with my device in portrait position, the application launches in portrait positioning and is not locked on rotation.

`{"orientation" : "portrait"}`as well doesn't seem to work on my Galaxy Nexus - when I launch the application with my device in landscape position, the application launches in landscape positioning and is not locked on rotation.

The same both apply with `{"orientation" : "landscape-primary"}` and `{"orientation" : "portrait-primary"}`.

Am I missing something here?

Comment 30

[Wesley Johnston (:wesj)]

Comment on attachment 647700 [details] [diff] [review]
Path 1/3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New stuffbr
User impact if declined: orientation in manifests don't work. 
Testing completed (on m-c, etc.): Landed today
Risk to taking this patch (and alternatives if risky): Medium risk. Some changes to our orientation support but nothing should be changed.
String or UUID changes made by this patch: None.

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 647700 [details] [diff] [review]
Path 1/3

Approving for Aurora due to need for webapp testing.

Comment 32

[Wesley Johnston (:wesj)]

(In reply to Aaron Train [:aaronmt] from comment #29)
> This doesn't seem to be working at all for me with the supplied property
> values in the manifest.
> 
> `{ "orientation" : "landscape" }` doesn't seem to work on my Galaxy Nexus —
> when I launch the application with my device in portrait position, the
> application launches in portrait positioning and is not locked on rotation.
> 
> `{"orientation" : "portrait"}`as well doesn't seem to work on my Galaxy
> Nexus - when I launch the application with my device in landscape position,
> the application launches in landscape positioning and is not locked on
> rotation.
> 
> The same both apply with `{"orientation" : "landscape-primary"}` and
> `{"orientation" : "portrait-primary"}`.
> 
> Am I missing something here?

Hmm.. not sure. I tested by hardcoding a value, but I will look tomorrow.

Comment 33

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27e1ecbbc6ed

Comment 34

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac946714323
https://hg.mozilla.org/releases/mozilla-aurora/rev/cdf595eaae64