Steps: 1. Go to testmanifest.com 2. Create a web app with more than one orientation property specified (e.g. portrait-primary,landscape-primary) 3. Install the app 4. Launch the app 5. Switch the orientation of the found to each orientation Expected: The orientation should lock to only the items called out in the comma-separated list. Other orientations cannot be accessed. Actual: A portrait-primary orientation is used instead no matter what configuration is used. Additional Notes: Also saw this similar issue when I provided illegal characters after the comma with portrait-primary,<illegal>landscape-secondary. Are we even handling the case for the comma-separated list?

The way this *should* work is as follows: By default, pages should be locked to the orientations enumerated in the manifest. I.e. if a page specifies "landscape, portrait-primary" then we should allow orienting in the landscape-primary, landscape-secondary and portrait-primary orientations. If a page then calls lockOrientation from javascript, then the value passed to that function should override what's in the manifest. Once the page calls unlockOrientation we should switch back to using the values from the manifest. Note that lockOrientation also takes a comma-separated list of orientations.

The DOM API doesn't allow a comma-separated list of orientations nor does the spec. I could fix that if we think that's needed. The Android backend isn't going to support all possibilities there but the Gonk backend might.

(In reply to Mounir Lamouri (:mounir) from comment #2) > The DOM API doesn't allow a comma-separated list of orientations nor does > the spec. ? The spec here - http://mozilla.github.com/webapps-spec/#optional-properties states that the orientation property uses a comma-separated list. We confirmed this in triage as well. It's in debate though right now to be considered to be an array instead, but that's a separate issue. > I could fix that if we think that's needed. The Android backend isn't going > to support all possibilities there but the Gonk backend might. Why wouldn't the Android backend support all of the possibilities? We recently landed support for the orientation property on Android.

(In reply to Jason Smith [:jsmith] from comment #3) > Why wouldn't the Android backend support all of the possibilities? We > recently landed support for the orientation property on Android. The API's Android gives us don't translate well over to this. Making it work involves basically rewriting things ourselves and trying to get information about the device that Android doesn't make easily available (AFAIK). Since supporting something like "portrait-secondary,landscape-secondary" or "portrait-primary,landscape-secondary" is a pretty rare thing, its not a priority.

(In reply to Jason Smith [:jsmith] from comment #3) > (In reply to Mounir Lamouri (:mounir) from comment #2) > > The DOM API doesn't allow a comma-separated list of orientations nor does > > the spec. > > ? The spec here - > http://mozilla.github.com/webapps-spec/#optional-properties states that the > orientation property uses a comma-separated list. We confirmed this in > triage as well. It's in debate though right now to be considered to be an > array instead, but that's a separate issue. No, I meant http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html Both specs have to match. I think we want to support this. However, I'm not sure if that *has* to be done for Basecamp. Depending on other priorities, I could work on that.

(In reply to Mounir Lamouri (:mounir) from comment #5) > (In reply to Jason Smith [:jsmith] from comment #3) > > (In reply to Mounir Lamouri (:mounir) from comment #2) > > > The DOM API doesn't allow a comma-separated list of orientations nor does > > > the spec. > > > > ? The spec here - > > http://mozilla.github.com/webapps-spec/#optional-properties states that the > > orientation property uses a comma-separated list. We confirmed this in > > triage as well. It's in debate though right now to be considered to be an > > array instead, but that's a separate issue. > > No, I meant > http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html > Both specs have to match. Oh, good point. Can you and Anant figure out how to resolve the spec differences here? > > I think we want to support this. However, I'm not sure if that *has* to be > done for Basecamp. Depending on other priorities, I could work on that. Fair enough, I agree. Putting this back on the nomination queue then.

I agree, this shouldn't block basecamp.

(In reply to Jason Smith [:jsmith] from comment #6) > (In reply to Mounir Lamouri (:mounir) from comment #5) > > (In reply to Jason Smith [:jsmith] from comment #3) > > > (In reply to Mounir Lamouri (:mounir) from comment #2) > > > > The DOM API doesn't allow a comma-separated list of orientations nor does > > > > the spec. > > > > > > ? The spec here - > > > http://mozilla.github.com/webapps-spec/#optional-properties states that the > > > orientation property uses a comma-separated list. We confirmed this in > > > triage as well. It's in debate though right now to be considered to be an > > > array instead, but that's a separate issue. > > > > No, I meant > > http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html > > Both specs have to match. > > Oh, good point. Can you and Anant figure out how to resolve the spec > differences here? Actually, dug into this with Anant. The screen orientation API does support a comma-separated list as per the wiki - https://wiki.mozilla.org/WebAPI/ScreenOrientation. Am I misreading something?

My recollection too was that we wanted a comma-separated list. But I admit that my memory could be more wishful thinking.

Yes indeed. I just specified what we had implemented given that going from one value to a list of value is backward-compatible.

I'm going to mute that bug in having lockOrientation taking an array.

Comment on attachment 657391 [details] [diff] [review] Patch >diff --git a/dom/interfaces/base/nsIDOMScreen.idl b/dom/interfaces/base/nsIDOMScreen.idl >--- a/dom/interfaces/base/nsIDOMScreen.idl >+++ b/dom/interfaces/base/nsIDOMScreen.idl >@@ -1,16 +1,16 @@ > /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsIDOMEventTarget.idl" > >-[scriptable, builtinclass, uuid(5a8294df-ffe4-48e5-803f-f57bebc29289)] >+[scriptable, builtinclass, uuid(01e8587b-35a9-4a59-8349-c7ee93846fb2)] > interface nsIDOMScreen : nsIDOMEventTarget > { > readonly attribute long top; > readonly attribute long left; > readonly attribute long width; > readonly attribute long height; > readonly attribute long pixelDepth; > readonly attribute long colorDepth; >@@ -25,16 +25,17 @@ interface nsIDOMScreen : nsIDOMEventTarg > * portrait-primary or portrait-secondary. > */ > readonly attribute DOMString mozOrientation; > > [implicit_jscontext] attribute jsval onmozorientationchange; > > /** > * Lock screen orientation to the specified type. >+ * The parameter can be a DOMString or an Array of DOMString. Nit: s/of DOMString/of DOMStrings/ or "DOMString's", if you prefer. But more generally, can we write a real comment here explaining what's going on, since the specs are still in flux? You could say something like Lock the screen to the specified orientations(s). This method returns true if the lock was acquired successfully, and false otherwise. The parameter can be a DOMString or an Array of DOMStrings. If you pass a string, we lock the screen to that one orientation. If you pass an Array, we ensure that the screen is always in one of the given orientations. Valid orientations are "portrait", "portrait-primary", "portrait-secondary", "landscape", "landscape-primary", and "landscape-secondary". These tokens are case-sensitive. If you pass a string that's not one of the valid orientations, or if you pass an array of orientations and any of the orientations in the array is not valid, we reject the lock and return false. The "-primary" orientations correspond to holding the device right-side up, while the "-secondary" orientations correspond to holding the device upside-down. Locking the orientation in "portrait" is the same as locking the orientation in ['portrait-primary', 'portrait-secondary'], and the "landscape" orientation similarly corresponds to the set ['landscape-primary', 'landscape-secondary']. >diff --git a/dom/base/ScreenOrientation.h b/dom/base/ScreenOrientation.h >--- a/dom/base/ScreenOrientation.h >+++ b/dom/base/ScreenOrientation.h >+typedef uint32_t ScreenOrientation; >+ >+static const ScreenOrientation eScreenOrientation_None = 0; >+static const ScreenOrientation eScreenOrientation_PortraitPrimary = 1; // 00000001 >+static const ScreenOrientation eScreenOrientation_PortraitSecondary = 2; // 00000010 >+static const ScreenOrientation eScreenOrientation_Portrait = 3; // 00000011 >+static const ScreenOrientation eScreenOrientation_LandscapePrimary = 4; // 00000100 >+static const ScreenOrientation eScreenOrientation_LandscapeSecondary = 8; // 00001000 >+static const ScreenOrientation eScreenOrientation_Landscape = 12; // 00001100 >+static const ScreenOrientation eScreenOrientation_EndGuard = 15; // 00001111 Presumably you don't need the end guard now that you're not using an enum? >diff --git a/dom/base/nsScreen.cpp b/dom/base/nsScreen.cpp >--- a/dom/base/nsScreen.cpp >+++ b/dom/base/nsScreen.cpp >@@ -8,16 +8,17 @@ > #include "nsIDocShell.h" > #include "nsPresContext.h" > #include "nsCOMPtr.h" > #include "nsDOMClassInfoID.h" > #include "nsIDocShellTreeItem.h" > #include "nsLayoutUtils.h" > #include "nsDOMEvent.h" > #include "nsGlobalWindow.h" >+#include "nsJSUtils.h" > > using namespace mozilla; > using namespace mozilla::dom; > > namespace { > > bool > IsChromeType(nsIDocShell *aDocShell) >@@ -325,35 +326,70 @@ nsScreen::GetMozOrientation(nsAString& a > aOrientation.AssignLiteral("landscape-secondary"); > break; > } > > return NS_OK; > } > > NS_IMETHODIMP >-nsScreen::MozLockOrientation(const nsAString& aOrientation, bool* aReturn) >+nsScreen::MozLockOrientation(const jsval& aOrientation, JSContext* aCx, bool* aReturn) I'm not too comfortable with the JSAPI, so someone else should double-check this method. >+ nsAutoTArray<nsString, 2> orientations; If you really care about performance here (it's not clear you do, but I remember you talking about this), you might as well make this 8 or something. Stack space is very cheap. >+ if (aOrientation.isString()) { >+ nsDependentJSString item; >+ item.init(aCx, aOrientation.toString()); >+ orientations.AppendElement(item); > } else { >- return NS_OK; >+ // We assume we have an array at that point. >+ NS_ENSURE_TRUE(aOrientation.isObject(), NS_ERROR_INVALID_ARG); We're not assuming anything -- in fact, we're doing a check. How about // If we don't have a string, we must have an object (in particular, an // Array). >+ JSObject& obj = aOrientation.toObject(); >+ uint32_t length; >+ NS_ENSURE_TRUE(JS_GetArrayLength(aCx, &obj, &length) && length > 0, >+ NS_ERROR_INVALID_ARG); >+ >+ orientations.SetLength(length); You want SetCapacity, not SetLength; SetLength will insert |length| empty strings at the front of the array. In fact, I'm surprised any tests pass with this... >+ for (uint32_t i=0; i<length; ++i) { i = 0; i < length; >+ jsval value; >+ NS_ENSURE_TRUE(JS_GetElement(aCx, &obj, i, &value), NS_ERROR_UNEXPECTED); >+ NS_ENSURE_TRUE(value.isString(), NS_ERROR_INVALID_ARG); >+ >+ nsDependentJSString item; >+ item.init(aCx, value); >+ orientations.AppendElement(item); >+ } >+ } >+ >+ ScreenOrientation orientation = eScreenOrientation_None; >+ >+ for (uint32_t i=0; i<orientations.Length(); ++i) { >+ nsString& item = orientations[i]; >+ >+ if (item.EqualsLiteral("portrait")) { >+ orientation |= eScreenOrientation_Portrait; It would be a lot clearer to me if we didn't have the plain Portrait/Landscape enum entries and had only the *{Primary,Secondary} entries. Then we could explicitly do an |or| here, and never use the non-primary/secondary enums anywhere else. That is, you'd do orientation |= eScreenOrientation_PortraitPrimary | eScreenOrientation_PortraitSecondary; At the moment, we have this weird situation where we have a "portrait" orientation but the screen's current orientation can never be "portrait"! >+ } else { >+ // If we don't know that the token, we should just return 'false' without >+ // throwing. s/know that/recognize/ >diff --git a/dom/base/test/test_screen_orientation.html b/dom/base/test/test_screen_orientation.html >--- a/dom/base/test/test_screen_orientation.html >+++ b/dom/base/test/test_screen_orientation.html Oh, it looks like you're not testing locking /to/ an array of strings. That probably explains why the test isn't failing due to the SetLength(). :) It's sad that we don't have any positive tests here. Can we add them and run them just on Android? Or can we give QA a manual test to run. Clearly there's the opportunity for bugs here.

What about a follow-up for portrait/landscape removal?

> What about a follow-up for portrait/landscape removal? sgtm

> NS_IMETHODIMP >-nsScreen::MozLockOrientation(const nsAString& aOrientation, bool* aReturn) >+nsScreen::MozLockOrientation(const jsval& aOrientation, JSContext* aCx, bool* aReturn) I'd still like someone who knows JSAPI to look over this, if you don't mind. >+ for (uint32_t i=0; i<length; ++i) { I commented on the spacing here in my previous review. r=me assuming that you tested this and it actually works this time, and that you get someone to look over the JSAPI bits for sanity.

The JSAPI bits have been reviewed by khuey during the MozCamp Europe.

Arrays appear to be recognized that contain singular entities into the app manifest for orientation, although the original intention behind this bug wasn't fixed - multiple orientations in the array still don't work and revert back to using portrait-primary when multiple orientations is used (example below). Mounir - Any ideas? { "name":"Test App ({subdomain})", "description":"This app has been automatically generated by testmanifest.com", "version":"1.0", "icons":{ "16":"http://testmanifest.com/icon-16.png", "48":"http://testmanifest.com/icon-48.png", "128":"http://testmanifest.com/icon-128.png" }, "installs_allowed_from":[ "*" ], "orientation": ["landscape-primary", "portrait-secondary"], "developer":{ "name":"Gregory Koberger", "url":"http://gkoberger.net" } }

This is not about manifest but about the Screen Orientation API. You should check that screen.mozLockOrientation() works with an array. It is only expected to work on Android. I don't think manifest handles that and do not expect this to work on B2G/Gaia.

(In reply to Mounir Lamouri (:mounir) from comment #21) > This is not about manifest but about the Screen Orientation API. You should > check that screen.mozLockOrientation() works with an array. It is only > expected to work on Android. I don't think manifest handles that and do not > expect this to work on B2G/Gaia. Ah okay. I transferred the original issue for the multiple orientations over to Gaia. If this is Android, then I'm unassigning myself for now (for context - we're targeting verifications primarily on basecamp+ blockers).

(In reply to Jason Smith [:jsmith] from comment #22) > (In reply to Mounir Lamouri (:mounir) from comment #21) > > This is not about manifest but about the Screen Orientation API. You should > > check that screen.mozLockOrientation() works with an array. It is only > > expected to work on Android. I don't think manifest handles that and do not > > expect this to work on B2G/Gaia. > > Ah okay. I transferred the original issue for the multiple orientations over > to Gaia. If this is Android, then I'm unassigning myself for now (for > context - we're targeting verifications primarily on basecamp+ blockers). One additional comment - or other areas that touch B2G specifically.

I'm getting mochitest errors with test_screen_orientation.html: http://people.mozilla.org/~mwargers/mochitestjs2/test_screen_orientation.html It complains about not exceptions being fired, when they should fire. But afaict, the spec says no exceptions should fire: http://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html#methods Also, I'm confused why tinderbox isn't red, because this mochitest is failing.

> http://people.mozilla.org/~mwargers/mochitestjs2/test_screen_orientation.html > It complains about not exceptions being fired, when they should fire. The test is green for me.

Ok, weird, it's now working also for me after a browser restart. I did some weird stuff beforehand, so a restart might have fixed this. The thing is that I'm still seeing it happening in b2g mochitest.

> The thing is that I'm still seeing it happening in b2g mochitest. Sounds like we should discuss this further in a new bug, then.

I guess that might be happening, because of using a too old build. And now that I'm saying that, the trunk build that I used was also too old! Sorry for the confusion.

Doc updated: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/18 and https://developer.mozilla.org/en-US/docs/Web/API/Screen.lockOrientation