Closed Bug 776600 Opened 8 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 17
Tracking Status
firefox16 --- fixed
firefox17 --- fixed
fennec - ---

People

(Reporter: jsmith, Assigned: wesj)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [blocking-webrtandroid1?])

Attachments

(3 files, 2 obsolete files)

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
Keywords: dev-doc-needed
Priority: -- → P1
Whiteboard: [blocking-webrtandroid1+]
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".
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?
Jonas - Do you know of a sample app that works on Firefox OS using the screen orientation manifest properties?
Attached patch WIP Patch (obsolete) — Splinter Review
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?
Attachment #646721 - Flags: feedback?(blassey.bugs)
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.
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Attached patch Path 1/3Splinter Review
This allows setting the default orientation, and creates a pref, read at startup, to set it.
Attachment #646721 - Attachment is obsolete: true
Attachment #646721 - Flags: feedback?(blassey.bugs)
Attachment #647700 - Flags: review?(blassey.bugs)
Attached patch Patch 2/3 (obsolete) — Splinter Review
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?
Attachment #647705 - Flags: review?(mark.finkle)
Attached patch PatchSplinter Review
Grr. Forgot to qref.
Attachment #647705 - Attachment is obsolete: true
Attachment #647705 - Flags: review?(mark.finkle)
Attachment #647707 - Flags: review?(mark.finkle)
This is the simplest way to add orientation support (I think?)
Attachment #647709 - Flags: review?(felipc)
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
Attachment #647709 - Flags: review?(felipc) → review+
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.
Whiteboard: [blocking-webrtandroid1+] → [blocking-webrtandroid1?]
No longer blocks: Blocking-FFA-WebRT1+
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
Attachment #647707 - Flags: review?(mark.finkle) → review+
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.
Attachment #647707 - Flags: approval-mozilla-aurora?
Flagging qawanted for testing the current changes landing.
Keywords: qawanted
This specifically doesn't effect fennec
tracking-fennec: ? → -
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
Attachment #647709 - Flags: approval-mozilla-aurora?
Comment on attachment 647707 [details] [diff] [review]
Patch

only affects webapps, approving for Aurora.
Attachment #647707 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #647709 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
blassey ping? I know you said you had questions about one of my patches. Can you drop them in here?
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
Attachment #647700 - Flags: review?(blassey.bugs) → review+
Note - I tried testing portrait as an app manifest property, but that did not work at all.
Keywords: qawanted
This has not all landed yet.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Whiteboard: [blocking-webrtandroid1?][leave open] → [blocking-webrtandroid1?]
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 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.
Attachment #647700 - Flags: approval-mozilla-aurora?
Target Milestone: --- → Firefox 17
Comment on attachment 647700 [details] [diff] [review]
Path 1/3

Approving for Aurora due to need for webapp testing.
Attachment #647700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
You need to log in before you can comment on or make changes to this bug.