support window.orientation and orientationchange event

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: hsteen, Assigned: wchen)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla44
Other
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [backout-asap][webvr], )

Attachments

(5 attachments, 3 obsolete attachments)

The lack of this feature breaks certain scripts and sites (see dependencies for details). It would be a good idea to fix this for site compatibility. (There is a workaround for Gecko that involves media queries, but it seems a bit too hackish to recommend widely.)

We should implement:
* window.orientation - takes values like 0, 90, -90, 180 depending on how the device is held

* orientationchange event that fires when the device is rotated

Test case: https://bugzilla.mozilla.org/attachment.cgi?id=805890
Note that we do support orientationchange as defined by the Screen Orientation spec (www.w3.org/TR/screen-orientation/) (at least, I think we do). I wonder if it would be possible to map the values that screen.orientation returns to window.orientation values that these scripts are expecting.
I wasn't aware of the Screen Orientation spec, but I don't think there's any support. At least this script running in Firefox Nightly on Android just outputs undefined:

document.write(window.screen.orientation);
window.screen.onorientationchange = function(){
    document.body.appendChild(document.createElement('p')).appendChild(document.createTextNode(window.screen.orientation));
}

Also, the spec's values seem less useful than the Safari approach:
http://www.w3.org/TR/screen-orientation/#dfn-current-orientation 

* portrait-primary, if the orientation is in the primary portrait mode;
* portrait-secondary, if the orientation is in the secondary portrait mode;
* landscape-primary, if the orientation is in the primary landscape mode;
* landscape-secondary, if the orientation is in the secondary landscape mode.

As far as I can tell, this doesn't tell you if the device was turned to the left or to the right. It's trivial to check which of width/height is larger and thus deduce what mode is "primary", so the W3C model seems good at providing superfluous information and lacking something that's actually useful..
Oops. It's screen.mozOrientation in our implementation.
I've done some bulk testing of websites considered important in China (plus a few random others), here's a list of sites that read window.orientation. There's probably more to come while I keep testing other countries.
(I should have said a list of *urls* - there are several URLs from the same site(s) in a few places. This run of 735ish sites turned up 77 URLs that read window.orientation)
I've also got some data from the top 64,621 domains (according to alexa), with all the JS inlined:

egrep -r "win(dow)?\.orientation" webdevdata.org-2013-12-09-064743/ > ~/Desktop/orientation.txt

$ cat ~/Desktop/orientation.txt | wc -l
5584

Unfortunately the file is ~62MB (probably due to gigantic single-line minified libs). But I've got some plans on how to make that more digestible.
Reporter

Updated

5 years ago
Blocks: 931905
Attaching a list of 75 sites whose iOS content reads the value of window.orientation at some point while loading.
Reporter

Updated

5 years ago
Blocks: 996435
(Ccing Anne because of [1])

I wrote a mapping between screen.mozOrientation and window.orientation in a Firefox for Android addon (described at [2]). I wonder if the screen orientation spec could get away with defining window.orientation in a similar way as a compatibility note.

[1] http://lists.w3.org/Archives/Public/public-webapps/2013OctDec/0372.html
[2] https://miketaylr.com/posts/2014/05/window-orientation-shim.html
Blink is willing to remove support for window.orientation if its usage is low enough. I pushed a usage metric recently but I do not think it made Chrome Stable yet. We hope to replace window.orientation usage with screen.orientation.
That's encouraging to hear. Looking forward to the results of the usage metrics in the wild.
The root cause for Bug 1013676 is they're using `window.onorientationchange != undefined` as a proxy for Desktop browsers (which we fall into, so search is broken).
Blocks: 1013676
Blocks: 1037222
Reporter

Updated

5 years ago
See Also: → 1064567
Re: 1064567, Microsoft is using window.orientation in their web properties (at least Excel) in addition to adding support to Windows Phone.
Blocks: 1115590
Duplicate of this bug: 1115592
Blocks: 1170774
William has implemented the new screen orientation API over in bug 1131470 and will help with this work, too.
Assignee: nobody → wchen
Cool!

I'll be speccing this work at https://compat.spec.whatwg.org (if you want to help write that as well, lmk ^_^).

I think we should be able to implement this in terms of Screen Orientation, even though that means we'll just have to ignore the problems pointed out by RichT here: 

<https://twitter.com/richtibbett/status/463370882560565248>. 

My thoughts were to just spec whatever the iPhone/iPad does, and ignore the billion wonky Android device behaviors.

Until I get to the spec bits, these might be useful:

https://miketaylr.com/posts/2014/05/window-orientation-shim.html
https://github.com/miketaylr/orientation-shim/blob/master/bootstrap.js#L13
Assignee

Comment 16

4 years ago
Here is a patch that implements window.orientation and window.onorientationchange to behave like it does on the android browser.

I noticed that the other browsers don't implement these properties on the desktop platform (they have undefined value), is this something we want to do as well? Ideally I'd only like to have this as minimally exposed as possible since I want authors to use screen.orientation because it behaves better (you don't get orientation change events when the page isn't active).

Also, if we have some kind of webpage rewrite mechanism for web compat already available, then maybe we should just map window.orientation to a slightly tweaked screen.orientation.angle and map the orientationchange event on the window to the orientationchange event on screen.orientation.
Hey William,

Thanks for the patch! I don't think there's any need to implement this on non-mobile platforms.

I think we'll need to support <body onorientationchange> in addition to window (cf. https://developer.apple.com/library/ios/documentation/AppleApplications/Reference/SafariWebContent/HandlingEvents/HandlingEvents.html#//apple_ref/doc/uid/TP40006511-SW16).

> Also, if we have some kind of webpage rewrite mechanism for web compat already available, then maybe we should just map window.orientation to a slightly tweaked screen.orientation.angle and map the orientationchange event on the window to the orientationchange event on screen.orientation.

I was planning on speccing this in terms of screen.orientation, but I haven't gotten there just quite yet: 

https://compat.spec.whatwg.org/#event-window-orientationchange just has IDL + first bits of event stuff. As for a "webpage rewrite mechanism for web compat" I have some ideas for Q4, but I'm not aware of anything just yet.
Assignee

Comment 19

4 years ago
Updated patch to remove API on non-mobile platforms and added support for <body onorientationchange>.

This patch is currently not implemented in terms of screen.orientation because it doesn't look like that's how the other browsers behave, but that can be changed if the differences don't matter.

If we're OK with the approach I've taken with this patch, we can get this reviewed and try to land it.
Attachment #8658852 - Attachment is obsolete: true
I say let's try to land--it doesn't necessarily need to be implemented in terms of screen.orientation, that was just the easiest way to polyfill it via JS.
Assignee

Comment 21

4 years ago
Attachment #8659007 - Attachment is obsolete: true
Attachment #8660974 - Flags: review?(amarchesini)
Comment on attachment 8660974 [details] [diff] [review]
Implement window.orienation and window.onorientationchange

Review of attachment 8660974 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm! can you just fix this comments and send it to me again? Ah! and add a test, please. Thanks!

::: dom/base/moz.build
@@ +129,5 @@
>      'nsTraversal.h',
>      'nsTreeSanitizer.h',
>      'nsViewportInfo.h',
>      'nsWindowMemoryReporter.h',
> +    'nsWindowOrientationObserver.h',

#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)

@@ +323,5 @@
>      'nsTraversal.cpp',
>      'nsTreeSanitizer.cpp',
>      'nsViewportInfo.cpp',
>      'nsWindowMemoryReporter.cpp',
> +    'nsWindowOrientationObserver.cpp',

#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)

::: dom/base/nsGlobalWindow.cpp
@@ +38,5 @@
>  #include "nsIController.h"
>  #include "nsScriptNameSpaceManager.h"
>  #include "nsISlowScriptDebug.h"
>  #include "nsWindowMemoryReporter.h"
> +#include "nsWindowOrientationObserver.h"

#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)

here and everywhere else.

@@ +1353,5 @@
>    if (ac)
>      ac->RemoveWindowAsListener(this);
>  
> +  if (mOrientationChangeObserver) {
> +    mOrientationChangeObserver->ClearWindow();

mOrientationChangeObserver = nullptr;

should be enough. Why do you need this ClearWindow()?

::: dom/base/nsGlobalWindow.h
@@ +92,5 @@
>  class nsGlobalWindow;
>  class nsDOMWindowUtils;
>  class nsIIdleService;
>  struct nsRect;
> +class nsWindowOrientationObserver;

move it before struct.

::: dom/base/nsWindowOrientationObserver.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

rename it WindowOrientationObserver.cpp/h

@@ +3,5 @@
> +/* 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 "nsWindowOrientationObserver.h"

remove 'ns'

@@ +17,5 @@
> + */
> +nsWindowOrientationObserver::nsWindowOrientationObserver(
> +  nsGlobalWindow* aGlobalWindow)
> +  : mWindow(aGlobalWindow)
> +{

MOZ_ASSERT(aGlobalWindow);

@@ +41,5 @@
> +{
> +  mWindow = nullptr;
> +}
> +
> +int16_t

/* static */ int16_t

::: dom/base/nsWindowOrientationObserver.h
@@ +21,5 @@
> +  void ClearWindow();
> +  static int16_t OrientationAngle();
> +
> +  // Weak pointer, instance is owned by mWindow.
> +  nsGlobalWindow* mWindow;

MOZ_NON_OWNING_REF

::: dom/events/EventListenerManager.cpp
@@ +461,5 @@
>        window->EnableDeviceSensor(SENSOR_ACCELERATION);
>        window->EnableDeviceSensor(SENSOR_LINEAR_ACCELERATION);
>        window->EnableDeviceSensor(SENSOR_GYROSCOPE);
>        break;
> +    case eOrientationChange:

#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)

@@ +494,5 @@
>        break;
>      case eDeviceLight:
>        window->DisableDeviceSensor(SENSOR_LIGHT);
>        break;
> +    case eOrientationChange:

#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)

::: widget/EventMessageList.h
@@ +360,5 @@
>  NS_EVENT_MESSAGE(eDeviceMotion,          eDeviceEventFirst + 1)
>  NS_EVENT_MESSAGE(eDeviceProximity,       eDeviceEventFirst + 2)
>  NS_EVENT_MESSAGE(eUserProximity,         eDeviceEventFirst + 3)
>  NS_EVENT_MESSAGE(eDeviceLight,           eDeviceEventFirst + 4)
> +NS_EVENT_MESSAGE(eOrientationChange,     eDeviceEventFirst + 5)

#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK) ?
Attachment #8660974 - Flags: review?(amarchesini) → review-
Was an intent to implement/ship sent for this?
Whiteboard: [webvr]
Assignee

Comment 24

4 years ago
Sorry for the delay. Addressed review comments.
Attachment #8660974 - Attachment is obsolete: true
Attachment #8675999 - Flags: review?(amarchesini)
Assignee

Comment 25

4 years ago
Posted patch v1 diff v2Splinter Review
Comment on attachment 8675999 [details] [diff] [review]
Implement window.orienation and window.onorientationchange v2

Review of attachment 8675999 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/WindowOrientationObserver.cpp
@@ +31,5 @@
> +void
> +WindowOrientationObserver::Notify(
> +  const mozilla::hal::ScreenConfiguration& aConfiguration)
> +{
> +  if (mWindow) {

why should this be null?

::: dom/base/WindowOrientationObserver.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class WindowOrientationObserver :

final?
Attachment #8675999 - Flags: review?(amarchesini) → review+
Assignee

Comment 28

4 years ago
This bug adds "orientation" as a property to window on mobile platform, thus causing some tests to fail. This patch renames the variables.
Attachment #8676538 - Flags: review?(seth)
William, will we ship this on desktop at some point?
Flags: needinfo?(wchen)
(Neither Safari or Chrome expose this on Desktop, so I think we shouldn't -- I should add a note to the spec).
Assignee

Comment 31

4 years ago
No. This isn't going to ship on desktop.
Flags: needinfo?(wchen)
Comment on attachment 8676538 [details] [diff] [review]
Update tests to avoid using orientation as variable name

Review of attachment 8676538 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!
Attachment #8676538 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/f8c8a7545833
https://hg.mozilla.org/mozilla-central/rev/b227e94179e2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1218439
The changes for this issue caused the smoketest blocker bug 1218439.  Please backout if possible.
Whiteboard: [webvr] → [backout-asap][webvr]

Comment 36

4 years ago
It looks like 1218439 is now fixed, so this patch should now stay in place?
Doesn't look like it was ever backed out.
You need to log in before you can comment on or make changes to this bug.