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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: vlad, Assigned: esawin)

Tracking

({dev-doc-needed})

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox46 fixed)

Details

(Whiteboard: [webvr])

Attachments

(2 attachments, 4 obsolete attachments)

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.
Whiteboard: [webvr]
Assignee: nobody → esawin
(Assignee)

Comment 1

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

(Based on bug 1073224)
Attachment #8699637 - Flags: review?(vladimir)
(Assignee)

Comment 2

3 years ago
Adds AbsoluteDeviceOrientation DOM events for applications requiring compass heading.

Do we want to put this behind a flag/pref?
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.
Attachment #8699637 - Flags: review?(vladimir) → review+
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.
(Assignee)

Updated

3 years ago
Depends on: 1073224
(Assignee)

Updated

3 years ago
Attachment #8699640 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Attachment #8699637 - Flags: review?(bugs)
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.
Attachment #8699640 - Flags: review?(bugs) → review+
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 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.
Attachment #8699637 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

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

Comment 10

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

Comment 11

3 years ago
Make the AbsoluteDeviceOrientation event Android-only.
Attachment #8699640 - Attachment is obsolete: true
Attachment #8705825 - Flags: review?(bugs)
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.)
Attachment #8705825 - Flags: review?(bugs) → review-
(Assignee)

Comment 13

3 years ago
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.
Attachment #8705825 - Attachment is obsolete: true
Attachment #8707443 - Flags: review?(bugs)
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.
Attachment #8707443 - Flags: review?(bugs) → review+
(Assignee)

Comment 15

3 years ago
(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.
Attachment #8707443 - Attachment is obsolete: true
Attachment #8709714 - Flags: review+
(Assignee)

Comment 17

3 years ago
Updated UUID.
Attachment #8709714 - Attachment is obsolete: true
Attachment #8710530 - Flags: review+
(Assignee)

Comment 18

3 years ago
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.
Keywords: dev-doc-needed

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ce8e6c5364c
https://hg.mozilla.org/mozilla-central/rev/94904a47150f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Duplicate of this bug: 1248560
You need to log in before you can comment on or make changes to this bug.