1205649 - Change device orientation to send relative events; add absolute device orientation event

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Chrome is going to change their behaviour to send relative orientation events by default in https://code.google.com/p/chromium/issues/detail?id=520546 to make it better compatible with both mobile Safari and with VR use cases.  We should do the same. (See https://github.com/w3c/deviceorientation/issues/21)

Additionally, they propose adding an 'absolutedeviceorientation' event that is guaranteed to be absolute.  Safari already has a 'webkitcompassheading' event for this.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1205649-1.1-Use-relative-orientation-for-DOM-Dev.patch

Switches default orientation sensor to GAME_ROTATION_VECTOR. With the fallback (on Android) added in bug 1073224 patch 3.

(Based on bug 1073224)

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1205649-2.1-Add-AbsoluteDeviceOrientation-DOM-ev.patch

Adds AbsoluteDeviceOrientation DOM events for applications requiring compass heading.

Do we want to put this behind a flag/pref?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8699637 [details] [diff] [review]
0001-Bug-1205649-1.1-Use-relative-orientation-for-DOM-Dev.patch

Patch looks fine to me, and I think we should do it, but we should see with MDN or evang folks what they think might break if we change the default.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8699640 [details] [diff] [review]
0002-Bug-1205649-2.1-Add-AbsoluteDeviceOrientation-DOM-ev.patch

You probably want a DOM peer to review this, but if we put anything behind a pref, I'd suggest we do what Chrome did -- if the pref is not set, keep everything the same as it is today (including using absolute orientation for device orientation); otherwise, change device orientation to use relative, and add the absolutedeviceorientation event.

However, if we can verify iPhone WebKit compat/equiv, and they expose absolutedeviceorientation, then I'd say we should just make the change, and not bother with a pref.

Comment 5

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

Comment on attachment 8699640 [details] [diff] [review]
0002-Bug-1205649-2.1-Add-AbsoluteDeviceOrientation-DOM-ev.patch


>+  if (aEventType.LowerCaseEqualsLiteral("absolutedeviceorientationevent")) {
>+    DeviceOrientationEventInit init;
>+    return DeviceOrientationEvent::Constructor(aOwner, EmptyString(), init);
>+  }
One shouldn't be able create new types of events using deprecated document.createEvent, so just
drop this.

Comment 6

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

Patches in bug 1073224 need better explanation what SENSOR_ROTATION_VECTOR, SENSOR_ORIENTATION and SENSOR_GAME_ROTATION_VECTOR mean, and how they differ from each others.
It is rather hard from https://bug1073224.bmoattachments.org/attachment.cgi?id=8705140 to understand what the new values are about.

Comment 7

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

Comment on attachment 8699637 [details] [diff] [review]
0001-Bug-1205649-1.1-Use-relative-orientation-for-DOM-Dev.patch

So I don't understand these new sensor types.

What is supposed to be enabled and when?
The patch in the other bug make android to enable SENSOR_ROTATION_VECTOR
and other platforms keep using SENSOR_ORIENTATION.

Then this patch changes that SENSOR_ROTATION_VECTOR usage to SENSOR_GAME_ROTATION_VECTOR.
dom/system/nsDeviceSensors.cpp then dispatch relative for TYPE_GAME_ROTATION_VECTOR, but absolute otherwise.

On non-Android, doesn't this break all deviceorientation usage?
You always dispatch absolutedeviceorientation (based on the patch 2) on those platforms, yet they have supported deviceorientation event for ages.

Comment 8

[Eugen Sawin [:esawin]]

(In reply to Olli Pettay [:smaug] from comment #7)
> So I don't understand these new sensor types.
> 
> What is supposed to be enabled and when?
> The patch in the other bug make android to enable SENSOR_ROTATION_VECTOR
> and other platforms keep using SENSOR_ORIENTATION.

Bug 1073224 fixes the DeviceOrientation support on Android by replacing a deprecated sensor type Sensor.TYPE_ORIENTATION with Sensor.TYPE_ROTATION_VECTOR.

Both sensor types generate absolute orientation events. TYPE_ROTATION_VECTOR fixes some issues (e.g., gimbal lock) but is still affected by drift.
 
> Then this patch changes that SENSOR_ROTATION_VECTOR usage to
> SENSOR_GAME_ROTATION_VECTOR.
> dom/system/nsDeviceSensors.cpp then dispatch relative for
> TYPE_GAME_ROTATION_VECTOR, but absolute otherwise.

In this bug, we're trying to address the drift problem by switching the default sensor type for DeviceOrientation to Sensor.TYPE_GAME_ROTATION_VECTOR which generates relative orientation events.

The spec supports both use cases [1] (via the "absolute" boolean field), but switching to relative orientation events by default without providing an option to request absolute device orientation would break support for applications requiring compass heading data. That's the reason for adding the new AbsoluteDeviceOrientation event type.
 
> On non-Android, doesn't this break all deviceorientation usage?
> You always dispatch absolutedeviceorientation (based on the patch 2) on
> those platforms, yet they have supported deviceorientation event for ages.

The patches should not change the behavior for DeviceOrientation events on non-Android devices.
However, I'll update patch 2 to have the same build flag guards for the AbsoluteDeviceOrientation event case, since not all platforms are guaranteed to provide the same sensor fallback mechanics as Fennec does. 

This changes mirror the (planned) behavior on Safari and Chrome [2].

[1] http://w3c.github.io/deviceorientation/spec-source-orientation.html
[2] https://code.google.com/p/chromium/issues/detail?id=520546

Comment 9

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

(In reply to Eugen Sawin [:esawin] from comment #8)
> The patches should not change the behavior for DeviceOrientation events on
> non-Android devices.
How so. without the patches FireDOMOrientationEvent dispatches always deviceorientation events, but
with the patch
       } else if (type == nsIDeviceSensorData::TYPE_ORIENTATION) {
        FireDOMOrientationEvent(target, x, y, z, Orientation::kAbsolute);
code path, which I assume is used on non-Android, will start dispatching absolutedeviceorientation.

Comment 10

[Eugen Sawin [:esawin]]

(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to Eugen Sawin [:esawin] from comment #8)
> > The patches should not change the behavior for DeviceOrientation events on
> > non-Android devices.
> How so. without the patches FireDOMOrientationEvent dispatches always
> deviceorientation events, but
> with the patch
>        } else if (type == nsIDeviceSensorData::TYPE_ORIENTATION) {
>         FireDOMOrientationEvent(target, x, y, z, Orientation::kAbsolute);
> code path, which I assume is used on non-Android, will start dispatching
> absolutedeviceorientation.

Oh, right, that's an issue with the second patch, I'm going to update that (I was confused because you r-ed patch 1).

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1205649-2.2-Add-AbsoluteDeviceOrientation-DOM-ev.patch

Make the AbsoluteDeviceOrientation event Android-only.

Comment 12

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

Comment on attachment 8705825 [details] [diff] [review]
0002-Bug-1205649-2.2-Add-AbsoluteDeviceOrientation-DOM-ev.patch

So if non-Android supports only absolute, we should dispatch 
absolutedeviceorientation on such platform *and* the old deviceorientation with 
.absolute true.
Given that it should be rather easy to just dispatch extra event in such case, better to do it now, and not have a version of gecko which dispatches only
absolute deviceorientation events, but not absolutedeviceorientation events.

That would follow the spirit of the current version of the spec more closely.

And nsIDeviceSensorData still really needs more documentation.
What data is relative what absolute and so on.
(apparently TYPE_ORIENTATION and TYPE_ROTATION_VECTOR are absolute but not sure how those are different. And TYPE_GAME_ROTATION_VECTOR is then about relative data.)

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1205649-2.3-Add-AbsoluteDeviceOrientation-DOM-ev.patch

This is the behavior of this patch version:
1. If only relative events are processed, then only relative "deviceorientation" events are dispatched.
2. If only absolute events are processed, then both, absolute "deviceorientation" and "absolutedeviceorientation" events are dispatched.
3. If both, relative and absolute events are processed, then relative "deviceorientation" and "absolutedeviceorientation" events are dispatched accordingly.

This provides backwards-compatibility while allowing a device with GAME_ROTATION_VECTOR support to receive both distinct event types.

Comment 14

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

Comment on attachment 8707443 [details] [diff] [review]
0002-Bug-1205649-2.3-Add-AbsoluteDeviceOrientation-DOM-ev.patch

>+++ b/dom/system/nsDeviceSensors.cpp
>@@ -390,17 +390,34 @@ nsDeviceSensors::FireDOMOrientationEvent(EventTarget* aTarget,
>   init.mGamma.SetValue(aGamma);
>   init.mAbsolute = aIsAbsolute;
> 
>-  RefPtr<DeviceOrientationEvent> event =
>-    DeviceOrientationEvent::Constructor(aTarget,
>-                                        NS_LITERAL_STRING("deviceorientation"),
>-                                        init);
>-  event->SetTrusted(true);
>+  auto dispatch = [&](const nsAString& aType) {
>+    RefPtr<DeviceOrientationEvent> event =
>+      DeviceOrientationEvent::Constructor(aTarget, aType, init);
>+    event->SetTrusted(true);
>+    bool dummy;
>+    aTarget->DispatchEvent(event, &dummy);
>+  };
whoohoo, possibly the first time ever I see C++ lambda usage which isn't totally insane. (DOM code is fortunately mostly lambda-free)
Nit, I'd put { to its own line under auto so that it follows the the coding style of normal functions and methods and not
control structures like 'if' or 'while'. Though looks like the few lambda usage we have in tree keeps { in the same line (against the coding style rules).
So either way.

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1205649-2.4-Add-AbsoluteDeviceOrientation-DOM-ev.patch

(In reply to Olli Pettay [:smaug] from comment #14)
> whoohoo, possibly the first time ever I see C++ lambda usage which isn't
> totally insane.

Apparently it is insane enough to trip off the static code checker:
"error: Refcounted variable 'aTarget' of type 'mozilla::dom::EventTarget' cannot be captured by a lambda"

I've moved "aTarget" to an argument of the lambda function to fix that.

> Nit, I'd put { to its own line under auto so that it follows the the coding
> style of normal functions and methods and not
> control structures like 'if' or 'while'. Though looks like the few lambda
> usage we have in tree keeps { in the same line (against the coding style
> rules).

I've reformatted the lambda to be compliant with function definition style (brackets, capitalization).

Carrying over r+ due to cosmetic changes only.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce8e6c5364c
https://hg.mozilla.org/integration/mozilla-inbound/rev/94904a47150f

Comment 17

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1205649-2.4-Add-AbsoluteDeviceOrientation-DOM-ev.patch

Updated UUID.

Comment 18

[Eugen Sawin [:esawin]]

MDN notes: 
1. Changed default for "deviceorientation" events on modern Android devices to relative events. This is within the spec (event contains an "absolute" property).
The default behavior is not documented, so no changes to the docs should be required.

2. New orientation event type "absolutedeviceorientation" for absolute orientation events. Same behavior and interface as "deviceorientation" but defaults to absolute events across platforms.
Needs documentation.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ce8e6c5364c
https://hg.mozilla.org/mozilla-central/rev/94904a47150f