Jason Smith [:jsmith] Reporter
	Description • 12 years ago

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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 1 • 12 years ago

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.

	Mounir Lamouri (:mounir) Assignee
	Comment 2 • 12 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 3 • 12 years ago

(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.

	Wesley Johnston (:wesj)
	Comment 4 • 12 years ago

(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.

	Mounir Lamouri (:mounir) Assignee
	Comment 5 • 12 years ago

(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.

	Jason Smith [:jsmith] Reporter
	Comment 6 • 12 years ago

(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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 12 years ago

I agree, this shouldn't block basecamp.

	Jason Smith [:jsmith] Reporter
	Comment 8 • 12 years ago

(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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 12 years ago

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

	Mounir Lamouri (:mounir) Assignee
	Comment 10 • 12 years ago

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

	Mounir Lamouri (:mounir) Assignee
	Comment 11 • 12 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 13 • 12 years ago

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.

	Mounir Lamouri (:mounir) Assignee
	Comment 14 • 12 years ago

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

	Justin Lebar (not reading bugmail)
	Comment 15 • 12 years ago

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

sgtm

	Justin Lebar (not reading bugmail)
	Comment 16 • 12 years ago

> 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.

	Mounir Lamouri (:mounir) Assignee
	Comment 17 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc30225833c4

	Mounir Lamouri (:mounir) Assignee
	Comment 18 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/bc30225833c4

	Jason Smith [:jsmith] Reporter
	Comment 20 • 12 years ago

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"
  }
}

	Mounir Lamouri (:mounir) Assignee
	Comment 21 • 12 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 22 • 12 years ago

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

	Jason Smith [:jsmith] Reporter
	Comment 23 • 12 years ago

(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.

	Martijn Wargers (dead)
	Comment 24 • 12 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 25 • 12 years ago

> 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.

	Martijn Wargers (dead)
	Comment 26 • 12 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 27 • 12 years ago

> 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.

	Martijn Wargers (dead)
	Comment 28 • 12 years ago

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.

	Jean-Yves Perrier [:teoli]
	Comment 29 • 11 years ago

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