The default bug view has changed. See this FAQ.

Implement the W3C DeviceOrientation Event Specification

VERIFIED FIXED in Firefox 6

Status

Fennec Graveyard
General
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: azakai, Assigned: azakai)

Tracking

({dev-doc-complete})

Trunk
Firefox 6
dev-doc-complete
Dependency tree / graph
Bug Flags:
in-litmus +

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Depends on: 485943
(Assignee)

Comment 1

6 years ago
Created attachment 494189 [details]
Test Page
(Assignee)

Comment 2

6 years ago
Created attachment 494591 [details]
Test Page (v2)
Attachment #494189 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 494606 [details]
Test Page (v3)
Attachment #494591 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Created attachment 494607 [details]
Test Page (v4)
(Assignee)

Updated

6 years ago
Attachment #494606 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 494614 [details] [diff] [review]
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.
Attachment #494614 - Flags: feedback?(doug.turner)

Comment 6

6 years ago
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.
Attachment #494614 - Flags: feedback?(doug.turner) → feedback+

Comment 7

6 years ago
oh, and tests, automated will be hard.... so just a test page that qa can use for litmus.
(Assignee)

Comment 8

6 years ago
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

6 years ago
I think you can just create a new event similar to what the existing code already does.
(Assignee)

Comment 10

6 years ago
A new event class as in new interface? So something like nsIDOMOrientationEventW3C.idl? Or just a new event instance in that loop there?
i think you can just do the new event instance.  I don't think here is a need for another idl.
(Assignee)

Comment 12

6 years ago
(I won't be able to get around to this bug for a while.)
Assignee: azakai → nobody

Updated

6 years ago
Attachment #494614 - Flags: review?(Olli.Pettay)
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.
Attachment #494614 - Flags: review?(Olli.Pettay) → review-
Could we try to get this fixed for FF5?
yes, i think we should.  alon, can you pick up?
(Assignee)

Comment 16

6 years ago
Sure.
Assignee: nobody → azakai
(Assignee)

Comment 17

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

Comment 18

6 years ago
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.
We're going to remove the old moz events eventually.
Removing them now might be ok. Doug, what do you think about?
clean cut is probably better.  alon, drop the old moz-specific event is fine.
(Assignee)

Comment 21

6 years ago
Created attachment 525586 [details] [diff] [review]
patch
Attachment #494614 - Attachment is obsolete: true
Attachment #525586 - Flags: review?(Olli.Pettay)
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
Attachment #525586 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 23

6 years ago
Created attachment 525744 [details] [diff] [review]
patch v2

Fixed patch.
Attachment #525586 - Attachment is obsolete: true
Attachment #525744 - Flags: review?(Olli.Pettay)
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 <dougt@dougt.org>
>+ * 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.
Attachment #525744 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 25

6 years ago
Created attachment 526762 [details] [diff] [review]
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?
Attachment #525744 - Attachment is obsolete: true
Attachment #526762 - Flags: review?(blassey.bugs)
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.
(Assignee)

Comment 27

6 years ago
Created attachment 526814 [details] [diff] [review]
patch v4

Got it, thanks. Here is a better test.
Attachment #526762 - Attachment is obsolete: true
Attachment #526814 - Flags: review?(blassey.bugs)
Attachment #526762 - Flags: review?(blassey.bugs)
(Assignee)

Comment 28

6 years ago
Created attachment 526816 [details] [diff] [review]
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.
Attachment #526816 - Flags: review+
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
Attachment #526814 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 30

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0ebf2860b252
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Verified:
Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2
Status: RESOLVED → VERIFIED
Flags: in-litmus?(nhirata.bugzilla)
Target Milestone: --- → Firefox 6
Alon, do you test whether new accelerometer works on non-Android such as Linux, Windows (Thinkpad) and cocoa (MacBook)?
(Assignee)

Comment 33

6 years ago
There was an apparent regression in those platforms, but dougt is addressing that in bug 662678 I believe.
(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.
Makoto - i will land the other patch tomorrow on M/C, and request approval to land for Aurora.
Please see bug 662678 for follow up to this bug which addresses some of the deviceorientation issues and implements the devicemotion event.
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
(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.
right.... it should have referenced 662678 -- which is basically a follow up to this bug.
http://hg.mozilla.org/releases/mozilla-aurora/rev/1f7a6005b6b0
status-firefox6: --- → fixed

Updated

6 years ago
Blocks: 667919
Depends on: 716173
litmus test case 50507
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/
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.
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.
Flags: in-litmus?(nhirata.bugzilla) → in-litmus+
Naoki, known. Check again after Bug 734324 lands.
Bug 734324 landed today.
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.