The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Web Apps
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jsmith, Assigned: wesj)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
Firefox 17
ARM
Android
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed, fennec-)

Details

(Whiteboard: [blocking-webrtandroid1?])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Keywords: dev-doc-needed
(Reporter)

Updated

5 years ago
Priority: -- → P1
Whiteboard: [blocking-webrtandroid1+]
(Reporter)

Updated

5 years ago
Blocks: 766259
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".
(Assignee)

Comment 2

5 years ago
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?
That sounds awesome!
(Reporter)

Comment 4

5 years ago
Jonas - Do you know of a sample app that works on Firefox OS using the screen orientation manifest properties?
Unfortunately not.
(Assignee)

Comment 6

5 years ago
Created attachment 646721 [details] [diff] [review]
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?
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.

Updated

5 years ago
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED

Updated

5 years ago
tracking-fennec: --- → ?
status-firefox16: --- → affected
status-firefox17: --- → affected
(Assignee)

Comment 8

5 years ago
Created attachment 647700 [details] [diff] [review]
Path 1/3

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)
(Assignee)

Comment 9

5 years ago
Created attachment 647705 [details] [diff] [review]
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?
Attachment #647705 - Flags: review?(mark.finkle)
(Assignee)

Comment 10

5 years ago
Created attachment 647707 [details] [diff] [review]
Patch

Grr. Forgot to qref.
Attachment #647705 - Attachment is obsolete: true
Attachment #647705 - Flags: review?(mark.finkle)
Attachment #647707 - Flags: review?(mark.finkle)
(Assignee)

Comment 11

5 years ago
Created attachment 647709 [details] [diff] [review]
Patch 3/3 - Webapps.jsm changes

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+
(Assignee)

Comment 13

5 years ago
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.
(Reporter)

Updated

5 years ago
Whiteboard: [blocking-webrtandroid1+] → [blocking-webrtandroid1?]
(Reporter)

Updated

5 years ago
No longer blocks: 766259
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+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/27de96f5f67a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd4e5c2fe0a
Whiteboard: [blocking-webrtandroid1?] → [blocking-webrtandroid1?][leave open]
(Assignee)

Comment 16

5 years ago
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?
(Reporter)

Comment 17

5 years ago
Flagging qawanted for testing the current changes landing.
Keywords: qawanted
https://hg.mozilla.org/mozilla-central/rev/6dd4e5c2fe0a
https://hg.mozilla.org/mozilla-central/rev/27de96f5f67a
This specifically doesn't effect fennec
tracking-fennec: ? → -
(Assignee)

Comment 20

5 years ago
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+
(Assignee)

Comment 22

5 years ago
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+
(Reporter)

Updated

5 years ago
Blocks: 783741
(Reporter)

Comment 24

5 years ago
Note - I tried testing portrait as an app manifest property, but that did not work at all.
Keywords: qawanted
(Assignee)

Comment 25

5 years ago
This has not all landed yet.
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0255e9c7f468
https://hg.mozilla.org/mozilla-central/rev/0255e9c7f468
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 28

5 years ago
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?
(Assignee)

Comment 30

5 years ago
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?

Updated

5 years ago
status-firefox17: affected → fixed
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+
(Assignee)

Comment 32

5 years ago
(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.
(Assignee)

Comment 33

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

Updated

5 years ago
status-firefox16: affected → fixed
(Assignee)

Comment 34

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac946714323
https://hg.mozilla.org/releases/mozilla-aurora/rev/cdf595eaae64
You need to log in before you can comment on or make changes to this bug.