615597 - Implement the W3C DeviceOrientation Event Specification

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alon Zakai (:azakai)]

Bug 485943 provided mozOrientation support. We should also support the W3C's draft spec for orientation and motion.

Spec: http://dev.w3.org/geo/api/spec-source-orientation.html

Comment 1

[Alon Zakai (:azakai)]

Attachment: Test Page

Comment 2

[Alon Zakai (:azakai)]

Attachment: Test Page (v2)

Comment 3

[Alon Zakai (:azakai)]

Attachment: Test Page (v3)

Comment 4

[Alon Zakai (:azakai)]

Attachment: Test Page (v4)

Comment 5

[Alon Zakai (:azakai)]

Attachment: First Stab

Ok, this sort of works, but I think I need some direction as to what the proper way to write the patch is.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 494614 [details] [diff] [review]
First Stab

this looks more or less fine.

I would document what alpha, beta, gamma are.  http://dev.w3.org/geo/api/spec-source-orientation.html will change.

do you need to create a new orientation event?  basically duplicate this block?

http://mxr.mozilla.org/mozilla-central/source/dom/system/nsAccelerometer.cpp#251



don't you have to fix up the other orientation implementations?  If so, lets factor that work out into different patches.

Comment 7

[Doug Turner (:dougt)]

oh, and tests, automated will be hard.... so just a test page that qa can use for litmus.

Comment 8

[Alon Zakai (:azakai)]

Should I create a separate W3C orientation event (if so what should it be called, to differentiate it from the existing one, nsIDOMOrientationEvent)? Or is it ok to use the same event for both as the patch does?

Comment 9

[Doug Turner (:dougt)]

I think you can just create a new event similar to what the existing code already does.

Comment 10

[Alon Zakai (:azakai)]

A new event class as in new interface? So something like nsIDOMOrientationEventW3C.idl? Or just a new event instance in that loop there?

Comment 11

[Doug Turner (:dougt)]

i think you can just do the new event instance.  I don't think here is a need for another idl.

Comment 12

[Alon Zakai (:azakai)]

(I won't be able to get around to this bug for a while.)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 494614 [details] [diff] [review]
First Stab

 
> // orientation support
> GK_ATOM(onMozOrientation, "onMozOrientation")
>+GK_ATOM(onDeviceOrientation, "ondeviceorientation")
The event name should have Moz prefix. But has the new event implemented
already without prefix in other engines? If so, I guess it is ok to
have the event without prefix in gecko too.


>@@ -52,20 +52,31 @@ NS_IMETHODIMP nsDOMOrientationEvent::Ini
>                                                           PRBool cancelableArg,
>                                                           double x,
>                                                           double y,
>                                                           double z)
> {
>   nsresult rv = nsDOMEvent::InitEvent(eventTypeArg, canBubbleArg, cancelableArg);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (fabs(x*x+y*y+z*z-1) > 0.1)
>+    return NS_ERROR_INVALID_ARG; // We expect a unit vector
>+
This is wrong.
A script should be able to initialize the even with any x/y/z parameters.
And btw, there are some spaces missing before and after '*' and '+'

The initFOO method doesn't look like the one in the draft specification.
The last parameter is missing.

>   mX = x;
>   mY = y;
>   mZ = z;
> 
>+  // Calculate W3C values - move from a vector to Euler-type angles
>+
>+  #define RADIANS_TO_DEGREES(a) (a*180/3.14159)
>+
>+  mAlpha = 0; // XX Should be null - we do not have enough information to figure this out
>+  mBeta = RADIANS_TO_DEGREES( asin(y) );
>+  mGamma = RADIANS_TO_DEGREES( asin(x) );
>+
What is this conversion?
Seems like we either need to support both the old Moz-event and the new
W3C event, or just the latter one. But certainly not mix them in a way which isn't 
compatible with the W3C draft specification.


> protected:
>-  double mX, mY, mZ;
>+  double mX, mY, mZ, mAlpha, mBeta, mGamma;
mAlpha, mBeta and mGamma aren't initialized anywhere.


And if we start to dispatch deviceorientation event, we should do it everywhere we dispatch
MozOrientation


Seems like W3C DeviceOrientationEvent has strange compassCalibrated which can't be
initialized if a script wants to manually create and initialize the event.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Could we try to get this fixed for FF5?

Comment 15

[Doug Turner (:dougt)]

yes, i think we should.  alon, can you pick up?

Comment 16

[Alon Zakai (:azakai)]

Sure.

Comment 17

[Alon Zakai (:azakai)]

> > // orientation support
> > GK_ATOM(onMozOrientation, "onMozOrientation")
> >+GK_ATOM(onDeviceOrientation, "ondeviceorientation")
> The event name should have Moz prefix. But has the new event implemented
> already without prefix in other engines? If so, I guess it is ok to
> have the event without prefix in gecko too.

Looks like this is supported on mobile safari and chrome,

http://stackoverflow.com/questions/4378435/how-to-access-accelerometer-gyroscope-data-from-javascript

> 
> 
> >@@ -52,20 +52,31 @@ NS_IMETHODIMP nsDOMOrientationEvent::Ini
> >                                                           PRBool cancelableArg,
> >                                                           double x,
> >                                                           double y,
> >                                                           double z)
> > {
> >   nsresult rv = nsDOMEvent::InitEvent(eventTypeArg, canBubbleArg, cancelableArg);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+  if (fabs(x*x+y*y+z*z-1) > 0.1)
> >+    return NS_ERROR_INVALID_ARG; // We expect a unit vector
> >+
> This is wrong.
> A script should be able to initialize the even with any x/y/z parameters.

It's an orientation event, [x,y,z] should be a unit vector, no? What is the meaning of say [2,0,0] as the input vector, compared to [1,0,0]?

> 
> The initFOO method doesn't look like the one in the draft specification.
> The last parameter is missing.
> 

I don't understand, are you saying that our InitOrientationEvent should be the same as initDeviceOrientationEvent in the draft? I may be missing something here. But they are different in other respects as well, for example x/y/z vs alpha/beta/gamma.

> >   mX = x;
> >   mY = y;
> >   mZ = z;
> > 
> >+  // Calculate W3C values - move from a vector to Euler-type angles
> >+
> >+  #define RADIANS_TO_DEGREES(a) (a*180/3.14159)
> >+
> >+  mAlpha = 0; // XX Should be null - we do not have enough information to figure this out
> >+  mBeta = RADIANS_TO_DEGREES( asin(y) );
> >+  mGamma = RADIANS_TO_DEGREES( asin(x) );
> >+
> What is this conversion?
> Seems like we either need to support both the old Moz-event and the new
> W3C event, or just the latter one. But certainly not mix them in a way which
> isn't 
> compatible with the W3C draft specification.
> 

The conversion here is from an x/y/z vector (what we have to work with) to alpha/beta/gamma Euler-style coordinates (what the W3C draft specifies).

Not sure why you say this is a mix. This is intended to implement the W3C draft (the part that we can implement so far).

> 
> And if we start to dispatch deviceorientation event, we should do it everywhere
> we dispatch
> MozOrientation
> 

Does the patch not do that? It dispatches deviceorientation in tandem with the code that dispatches MozOrientation.

Comment 18

[Alon Zakai (:azakai)]

How badly do we want to keep the old moz-specific orientation events? It would be much simpler to write this patch assuming we remove those, and since we will be removing them at some point anyhow, I'd like to do it now.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

We're going to remove the old moz events eventually.
Removing them now might be ok. Doug, what do you think about?

Comment 20

[Doug Turner (:dougt)]

clean cut is probably better.  alon, drop the old moz-specific event is fine.

Comment 21

[Alon Zakai (:azakai)]

Attachment: patch

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 525586 [details] [diff] [review]
patch

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -1688,17 +1688,17 @@ GK_ATOM(onMozTapGesture, "onMozTapGestur
> GK_ATOM(onMozPressTapGesture, "onMozPressTapGesture")
> 
> // Touch events
> GK_ATOM(onMozTouchDown, "onMozTouchDown")
> GK_ATOM(onMozTouchMove, "onMozTouchMove")
> GK_ATOM(onMozTouchUp, "onMozTouchUp")
> 
> // orientation support
>-GK_ATOM(onMozOrientation, "onMozOrientation")
>+GK_ATOM(onDeviceOrientation, "ondeviceorientation")

Please name this ondeviceorientation


>diff --git a/content/events/src/nsDOMOrientationEvent.cpp b/content/events/src/nsDOMOrientationEvent.cpp
>--- a/content/events/src/nsDOMOrientationEvent.cpp
>+++ b/content/events/src/nsDOMOrientationEvent.cpp
>@@ -42,58 +42,73 @@ NS_IMPL_RELEASE_INHERITED(nsDOMOrientati
> 
> DOMCI_DATA(OrientationEvent, nsDOMOrientationEvent)
> 
> NS_INTERFACE_MAP_BEGIN(nsDOMOrientationEvent)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMOrientationEvent)
>   NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(OrientationEvent)
> NS_INTERFACE_MAP_END_INHERITING(nsDOMEvent)

This all isn't right. The interface name is DeviceOrientationEvent, not OrientationEvent.
If you type javascript: DeviceOrientationEvent to the location bar,
that should give [object DeviceOrientationEvent] as a result.


>+NS_IMETHODIMP nsDOMOrientationEvent::InitDeviceOrientationEvent(const nsAString & eventTypeArg,
>+                                                                PRBool canBubbleArg,
>+                                                                PRBool cancelableArg,
>+                                                                double alpha,
>+                                                                double beta,
>+                                                                double gamma,
>+                                                                PRBool absolute)
While you're here, could you fix the coding style.
Parameters should be in form aParameter



>-  mX = x;
>-  mY = y;
>-  mZ = z;
>+  mAlpha = alpha;
>+  mBeta = beta;
>+  mGamma = gamma;
You're not storing absolute anywhere


>+NS_IMETHODIMP nsDOMOrientationEvent::GetCompassCalibrated(PRBool *aCompassCalibrated)
>+{
>+  NS_ENSURE_ARG_POINTER(aCompassCalibrated);
>+
>+  *aCompassCalibrated = PR_TRUE;
>+  return NS_OK;
This is interesting. There is no way per spec one could set this value.
So per spec this is right (well, I'm not sure about whether to return PR_FALSE or PR_TRUE),
but IMO the spec is just buggy. I'll send email to the list or discuss with Andrei.


> 
> [scriptable, uuid(1618546a-c176-40a2-9086-2d973acceeb1)]
FYI, if you're changing an interface, you must update its uuid


> interface nsIDOMOrientationEvent : nsIDOMEvent
...but in this case we want a new interface called nsIDOMDeviceOrientationEvent

Comment 23

[Alon Zakai (:azakai)]

Attachment: patch v2

Fixed patch.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 525744 [details] [diff] [review]
patch v2

>diff --git a/dom/interfaces/events/Makefile.in b/dom/interfaces/events/Makefile.in
>--- a/dom/interfaces/events/Makefile.in
>+++ b/dom/interfaces/events/Makefile.in
>@@ -77,17 +77,17 @@ XPIDLSRCS =					\
> 	nsIDOMMessageEvent.idl			\
> 	nsIDOMNotifyPaintEvent.idl              \
> 	nsIDOMNotifyAudioAvailableEvent.idl     \
> 	nsIDOMPaintRequest.idl			\
> 	nsIDOMPaintRequestList.idl		\
> 	nsIDOMSimpleGestureEvent.idl		\
> 	nsIDOMNSMouseEvent.idl			\
> 	nsIDOMMozTouchEvent.idl			\
>-	nsIDOMOrientationEvent.idl              \
>+	nsIDOMDeviceOrientationEvent.idl              \
In this case one should use tabs not spaces to align \ properly.


>diff --git a/dom/interfaces/events/nsIDOMDeviceOrientationEvent.idl b/dom/interfaces/events/nsIDOMDeviceOrientationEvent.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/interfaces/events/nsIDOMDeviceOrientationEvent.idl
>@@ -0,0 +1,76 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Doug Turner <[e-mail]>
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
Doesn't look right. Initial developer should be
Mozilla Foundation and it is 2011

> [scriptable, uuid(1B406E32-CF42-471E-A470-6FD600BF4C7B)]
> interface nsIAcceleration : nsISupport
you must update uuid when you change an interface.


Please add some tests, at least to creating deviceorientation event and initializing it.


Someone else needs to look at the android part.

Comment 25

[Alon Zakai (:azakai)]

Attachment: patch v3

1. blassey, can you please review the Android bits here?

2. smaug, I added a test to check for the existence of DeviceOrientationEvent. I'm not sure what else you would like tested here, though?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Test that document.createEvent("DeviceOrientationEvent");
works and that you can initialize and dispatch that event.
And that event listener listening for the event is called
when the event is dispatched.

Comment 27

[Alon Zakai (:azakai)]

Attachment: patch v4

Got it, thanks. Here is a better test.

Comment 28

[Alon Zakai (:azakai)]

Attachment: diff between v3 and v4

smaug, if you want to take a look, this is the difference between patch 3 and patch 4 that I just posted.

Comment 29

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

Comment on attachment 526814 [details] [diff] [review]
patch v4

Review of attachment 526814 [details] [diff] [review]:

just reviewed the android bits, biggest change is to use doubles rather than floats.

::: embedding/android/GeckoEvent.java
@@ +95,5 @@
     public int mAction;
     public long mTime;
     public Point mP0, mP1;
     public Rect mRect;
+    public float mAlpha, mBeta, mGamma;

these should be doubles. The idl has double values and toDegrees() (see below) returns doubles

@@ +147,5 @@
+        // by comparing the magnitude to SensorManager.GRAVITY_EARTH, but
+        // don't have any easy way to use that information...
+        float magnitude = FloatMath.sqrt(s.values[0]*s.values[0] +
+                                         s.values[1]*s.values[1] + 
+                                         s.values[2]*s.values[2]);

put a space before and after the *'s

@@ +151,5 @@
+                                         s.values[2]*s.values[2]);
+        final float RADIANS_TO_DEGREES = 180f/3.14159f;
+        mAlpha = 0; // This should be null; we do not have enough info to calculate it
+        mBeta = (float)Math.asin(s.values[1] / magnitude) * RADIANS_TO_DEGREES;
+        mGamma = -(float)Math.asin(s.values[0] / magnitude) * RADIANS_TO_DEGREES;

use Math.toDegrees() (http://developer.android.com/reference/java/lang/Math.html#toDegrees%28double%29) and get rid of RADIANS_TO_DEGREES

Also, if you need Pi in the future, you can use Math.PI

::: widget/src/android/AndroidJavaWrappers.cpp
@@ +143,5 @@
     jP0Field = getField("mP0", "Landroid/graphics/Point;");
     jP1Field = getField("mP1", "Landroid/graphics/Point;");
+    jAlphaField = getField("mAlpha", "F");
+    jBetaField = getField("mBeta", "F");
+    jGammaField = getField("mGamma", "F");

doubles here

@@ +400,5 @@
 
         case SENSOR_EVENT:
+            mAlpha = jenv->GetFloatField(jobj, jAlphaField);
+            mBeta = jenv->GetFloatField(jobj, jBetaField);
+            mGamma = jenv->GetFloatField(jobj, jGammaField);

doubles here

::: widget/src/android/AndroidJavaWrappers.h
@@ +393,5 @@
     const nsIntPoint& P0() { return mP0; }
     const nsIntPoint& P1() { return mP1; }
+    float Alpha() { return mAlpha; }
+    float Beta() { return mBeta; }
+    float Gamma() { return mGamma; }

return doubles here

@@ +421,5 @@
     int mKeyCode, mUnicodeChar;
     int mOffset, mCount;
     int mRangeType, mRangeStyles;
     int mRangeForeColor, mRangeBackColor;
+    float mAlpha, mBeta, mGamma;

and finally doubles here too

Comment 30

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/0ebf2860b252

Comment 31

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified:
Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2

Comment 32

[Makoto Kato [:m_kato]]

Alon, do you test whether new accelerometer works on non-Android such as Linux, Windows (Thinkpad) and cocoa (MacBook)?

Comment 33

[Alon Zakai (:azakai)]

There was an apparent regression in those platforms, but dougt is addressing that in bug 662678 I believe.

Comment 34

[Makoto Kato [:m_kato]]

(In reply to comment #33)
> There was an apparent regression in those platforms, but dougt is addressing
> that in bug 662678 I believe.

This is included in Aurora, so should you back out this into aurora/beta if Doug doesn't fix it by 6.0??

Hirata-san, you should test this on MacBook Pro, ThinkPad and Maemo (GTK and Qt).  VERIFIED is incomplete on current situation.

Comment 35

[Doug Turner (:dougt)]

Makoto - i will land the other patch tomorrow on M/C, and request approval to land for Aurora.

Comment 36

[Doug Turner (:dougt)]

Please see bug 662678 for follow up to this bug which addresses some of the deviceorientation issues and implements the devicemotion event.

Comment 37

[Mounir Lamouri (:mounir)]

Doug, you pushed that patch to m-c with this bug number but the patch isn't attached here, is that expected?
http://hg.mozilla.org/mozilla-central/rev/d48792c8de67

Comment 38

[Mounir Lamouri (:mounir)]

(In reply to comment #37)
> Doug, you pushed that patch to m-c [...]

Sorry, I meant m-i and I merged it to m-c.

Comment 39

[Doug Turner (:dougt)]

right.... it should have referenced 662678 -- which is basically a follow up to this bug.

Comment 40

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/1f7a6005b6b0

Comment 41

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

litmus test case 50507

Comment 42

[Doug Turner (:dougt)]

that litmus test is a start, but its anything more that the most basic smoke test.


How about expanding it by using:
  http://dl.dropbox.com/u/8727858/physical-events/index.html

Have you looked into the EU html5 tests?
  http://html5.mosquito-fp7.eu/

Comment 43

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Fair point and great suggestion.  Each rotational axis, acceleration, rotational rate and interval rate (sample rate from device) testing is needed.

50517, 50518, 50519, 50520, 50521, 50522, 50523; need to run through each test to complete the test cases.  Leaving assigned litmus to myself until complete.

Comment 44

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

There are certain bugs that are not fixed as of yet; rotational rate, acceleration rate, interval rate, and compassneedscalibration.

Orientation.angle + accelerationIncludingGravity does seem to work.

Comment 45

[Doug Turner (:dougt)]

Naoki, known. Check again after Bug 734324 lands.

Comment 46

[Martijn Wargers (dead)]

Bug 734324 landed today.

Comment 47

[Jeremie Patonnier :Jeremie]

All the related documentation is available here:

https://developer.mozilla.org/en-US/docs/tag/Device%20Orientation

More specifically:

https://developer.mozilla.org/en-US/docs/Web/API/DeviceOrientationEvent
https://developer.mozilla.org/en-US/docs/Web/API/DeviceMotionEvent
https://developer.mozilla.org/en-US/docs/WebAPI/Detecting_device_orientation