support window.orientation and orientationchange event

RESOLVED FIXED in Firefox 44

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: hallvors, Assigned: wchen)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
mozilla44
Other
Android
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

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

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 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
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.
(Reporter)

Comment 2

5 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..
Oops. It's screen.mozOrientation in our implementation.
(Reporter)

Comment 4

4 years ago
Created attachment 8358232 [details]
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.
(Reporter)

Comment 5

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

4 years ago
Blocks: 931905
(Reporter)

Comment 7

4 years ago
Created attachment 8392009 [details]
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.
(Reporter)

Updated

4 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
Keywords: dev-doc-needed
(Reporter)

Updated

4 years ago
See Also: → bug 1064567
Re: 1064567, Microsoft is using window.orientation in their web properties (at least Excel) in addition to adding support to Windows Phone.

Updated

3 years ago
Duplicate of this bug: 1115592
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

3 years ago
Created attachment 8658852 [details] [diff] [review]
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.
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

3 years ago
Created attachment 8659007 [details] [diff] [review]
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.
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

3 years ago
Created attachment 8660974 [details] [diff] [review]
Implement window.orienation and window.onorientationchange
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

3 years ago
Created attachment 8675999 [details] [diff] [review]
Implement window.orienation and window.onorientationchange v2

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

Comment 25

3 years ago
Created attachment 8676000 [details] [diff] [review]
v1 diff v2
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

3 years ago
Created attachment 8676538 [details] [diff] [review]
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.
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

3 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: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

3 years ago
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

3 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.
Blocks: 1236928
You need to log in before you can comment on or make changes to this bug.