Closed
Bug 920734
Opened 12 years ago
Closed 9 years ago
support window.orientation and orientationchange event
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: hsteen, Assigned: wchen)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-needed, Whiteboard: [backout-asap][webvr])
Attachments
(5 files, 3 obsolete files)
3.53 KB,
text/csv
|
Details | |
1.41 KB,
text/plain
|
Details | |
19.09 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
17.89 KB,
patch
|
Details | Diff | Splinter Review | |
10.04 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
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•12 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.
Reporter | ||
Comment 2•12 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..
Comment 3•12 years ago
|
||
Oops. It's screen.mozOrientation in our implementation.
Reporter | ||
Comment 4•11 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.
Reporter | ||
Comment 5•11 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)
Comment 6•11 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.
Reporter | ||
Comment 7•11 years ago
|
||
Attaching a list of 75 sites whose iOS content reads the value of window.orientation at some point while loading.
Comment 8•11 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
Comment 9•11 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.
Comment 10•11 years ago
|
||
That's encouraging to hear. Looking forward to the results of the usage metrics in the wild.
Comment 11•11 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).
Blocks: 1013676
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 12•11 years ago
|
||
Re: 1064567, Microsoft is using window.orientation in their web properties (at least Excel) in addition to adding support to Windows Phone.
Comment 14•10 years ago
|
||
William has implemented the new screen orientation API over in bug 1131470 and will help with this work, too.
Assignee: nobody → wchen
Comment 15•10 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
Assignee | ||
Comment 16•10 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.
Comment 17•10 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.
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 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
Comment 20•10 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.
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8659007 -
Attachment is obsolete: true
Attachment #8660974 -
Flags: review?(amarchesini)
Comment 22•10 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) ?
Attachment #8660974 -
Flags: review?(amarchesini) → review-
Comment 23•10 years ago
|
||
Was an intent to implement/ship sent for this?
Updated•10 years ago
|
Whiteboard: [webvr]
Assignee | ||
Comment 24•10 years ago
|
||
Sorry for the delay. Addressed review comments.
Attachment #8660974 -
Attachment is obsolete: true
Attachment #8675999 -
Flags: review?(amarchesini)
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 years ago
|
||
Comment 27•10 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?
Attachment #8675999 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 28•10 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)
Comment 29•9 years ago
|
||
William, will we ship this on desktop at some point?
Flags: needinfo?(wchen)
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).
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!
Attachment #8676538 -
Flags: review?(seth) → review+
Comment 33•9 years ago
|
||
Comment 34•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f8c8a7545833
https://hg.mozilla.org/mozilla-central/rev/b227e94179e2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 35•9 years ago
|
||
The changes for this issue caused the smoketest blocker bug 1218439. Please backout if possible.
Whiteboard: [webvr] → [backout-asap][webvr]
Comment 36•9 years ago
|
||
It looks like 1218439 is now fixed, so this patch should now stay in place?
Comment 37•9 years ago
|
||
Doesn't look like it was ever backed out.
Updated•9 years ago
|
Blocks: compat-spec
You need to log in
before you can comment on or make changes to this bug.
Description
•