920734 - support window.orientation and orientationchange event

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

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

Comment 1

[Mike Taylor [:miketaylr]]

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.

Comment 2

[Hallvord R. M. Steen [:hallvors]]

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..

Comment 3

[Mike Taylor [:miketaylr]]

Oops. It's screen.mozOrientation in our implementation.

Comment 4

[Hallvord R. M. Steen [:hallvors]]

Attachment: window.orientation-usage-china.csv

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.

Comment 5

[Hallvord R. M. Steen [:hallvors]]

(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)

Comment 6

[Mike Taylor [:miketaylr]]

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.

Comment 7

[Hallvord R. M. Steen [:hallvors]]

Attachment: window-orientation.txt - data from general test run of sites tracked on AWCY in January

Attaching a list of 75 sites whose iOS content reads the value of window.orientation at some point while loading.

Comment 8

[Mike Taylor [:miketaylr]]

(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

Comment 9

[Mounir Lamouri (:mounir)]

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.

Comment 10

[Mike Taylor [:miketaylr]]

That's encouraging to hear. Looking forward to the results of the usage metrics in the wild.

Comment 11

[Mike Taylor [:miketaylr]]

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).

Comment 12

[Mike Taylor [:miketaylr]]

Re: 1064567, Microsoft is using window.orientation in their web properties (at least Excel) in addition to adding support to Windows Phone.

Comment 14

[Andrew Overholt [:overholt]]

William has implemented the new screen orientation API over in bug 1131470 and will help with this work, too.

Comment 15

[Mike Taylor [:miketaylr]]

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

Comment 16

[William Chen [:wchen]]

Attachment: Implement window.orienation and window.onorientationchange.

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.

Comment 17

[Mike Taylor [:miketaylr]]

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.

Comment 18

[Mike Taylor [:miketaylr]]

Correct url, https://compat.spec.whatwg.org/#windoworientation-interface

Comment 19

[William Chen [:wchen]]

Attachment: Implement window.orienation and window.onorientationchange.

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.

Comment 20

[Mike Taylor [:miketaylr]]

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.

Comment 21

[William Chen [:wchen]]

Attachment: Implement window.orienation and window.onorientationchange

Comment 22

[Andrea Marchesini [:baku]]

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) ?

Comment 23

[Mounir Lamouri (:mounir)]

Was an intent to implement/ship sent for this?

Comment 24

[William Chen [:wchen]]

Attachment: Implement window.orienation and window.onorientationchange v2

Sorry for the delay. Addressed review comments.

Comment 25

[William Chen [:wchen]]

Attachment: v1 diff v2

Comment 26

[William Chen [:wchen]]

Intent to ship: https://groups.google.com/d/msg/mozilla.dev.platform/N3i7Yc2UpR0/huNQI2fVBQAJ

Comment 27

[Andrea Marchesini [:baku]]

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?

Comment 28

[William Chen [:wchen]]

Attachment: Update tests to avoid using orientation as variable name

This bug adds "orientation" as a property to window on mobile platform, thus causing some tests to fail. This patch renames the variables.

Comment 29

[Andrew Overholt [:overholt]]

William, will we ship this on desktop at some point?

Comment 30

[Mike Taylor [:miketaylr]]

(Neither Safari or Chrome expose this on Desktop, so I think we shouldn't -- I should add a note to the spec).

Comment 31

[William Chen [:wchen]]

No. This isn't going to ship on desktop.

Comment 32

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8676538 [details] [diff] [review]
Update tests to avoid using orientation as variable name

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

LGTM!

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c8a7545833
https://hg.mozilla.org/integration/mozilla-inbound/rev/b227e94179e2

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8c8a7545833
https://hg.mozilla.org/mozilla-central/rev/b227e94179e2

Comment 35

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

The changes for this issue caused the smoketest blocker bug 1218439.  Please backout if possible.

Comment 36

[Casey Yee]

It looks like 1218439 is now fixed, so this patch should now stay in place?

Comment 37

[Andrew Overholt [:overholt]]

Doesn't look like it was ever backed out.