Hallvord R. M. Steen [:hallvors] Reporter
	Description • 11 years ago

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

	Mike Taylor [:miketaylr]
	Comment 1 • 11 years ago

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.

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 2 • 11 years ago

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

	Mike Taylor [:miketaylr]
	Comment 3 • 11 years ago

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

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 4 • 10 years ago

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.

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 5 • 10 years ago

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

	Mike Taylor [:miketaylr]
	Comment 6 • 10 years ago

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.

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 7 • 10 years ago

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

	Mike Taylor [:miketaylr]
	Comment 8 • 10 years ago

(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

	Mounir Lamouri (:mounir)
	Comment 9 • 10 years ago

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.

	Mike Taylor [:miketaylr]
	Comment 10 • 10 years ago

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

	Mike Taylor [:miketaylr]
	Comment 11 • 10 years ago

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

	Mike Taylor [:miketaylr]
	Comment 12 • 10 years ago

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

	Andrew Overholt [:overholt]
	Comment 14 • 9 years ago

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

	Mike Taylor [:miketaylr]
	Comment 15 • 9 years ago

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

	William Chen [:wchen] Assignee
	Comment 16 • 9 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.

	Mike Taylor [:miketaylr]
	Comment 17 • 9 years ago

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.

	Mike Taylor [:miketaylr]
	Comment 18 • 9 years ago

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

	William Chen [:wchen] Assignee
	Comment 19 • 9 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.

	Mike Taylor [:miketaylr]
	Comment 20 • 9 years ago

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.

	Andrea Marchesini [:baku]
	Comment 22 • 9 years ago

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

	Mounir Lamouri (:mounir)
	Comment 23 • 9 years ago

Was an intent to implement/ship sent for this?

	William Chen [:wchen] Assignee
	Comment 24 • 9 years ago

Sorry for the delay. Addressed review comments.

	William Chen [:wchen] Assignee
	Comment 26 • 9 years ago

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

	Andrea Marchesini [:baku]
	Comment 27 • 9 years ago

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?

	William Chen [:wchen] Assignee
	Comment 28 • 9 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.

	Andrew Overholt [:overholt]
	Comment 29 • 9 years ago

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

	Mike Taylor [:miketaylr]
	Comment 30 • 9 years ago

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

	William Chen [:wchen] Assignee
	Comment 31 • 9 years ago

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

	Seth Fowler [:seth] [:s2h]
	Comment 32 • 9 years ago

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

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

LGTM!

	Pulsebot
	Comment 33 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 34 • 9 years ago

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

	Jayme Mercado [:JMercado] (Inactive 3/4/2016)
	Comment 35 • 9 years ago

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

	Casey Yee
	Comment 36 • 9 years ago

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

	Andrew Overholt [:overholt]
	Comment 37 • 9 years ago

Doesn't look like it was ever backed out.