Bug 102869 – Implement the Screen Orientation API

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryuan Choi]

Adding screen orientation

[Alexey Proskuryakov]

Has this feature been announced on webkit-dev? I couldn't quickly find such an e-mail.

[Ryuan Choi]

(In reply to comment #1)
> Has this feature been announced on webkit-dev? I couldn't quickly find such an e-mail.

Sorry for the late. I announced now.

[Ryuan Choi]

Created attachment 228239 [details]
initial version

[Ryuan Choi]

*** Bug 125914 has been marked as a duplicate of this bug. ***

[Build Bot]

Comment on attachment 228239 [details]
initial version

Attachment 228239 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5073369552650240

New failing tests:
screen_orientation/screenorientation-api.html
screen_orientation/consecutive-locking.html
screen_orientation/orientation-change-event.html

[Build Bot]

Created attachment 228242 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228239 [details]
initial version

Attachment 228239 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6228977748279296

New failing tests:
screen_orientation/screenorientation-api.html
screen_orientation/consecutive-locking.html
screen_orientation/orientation-change-event.html

[Build Bot]

Created attachment 228247 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228239 [details]
initial version

Attachment 228239 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4550764374523904

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
screen_orientation/screenorientation-api.html
screen_orientation/consecutive-locking.html
screen_orientation/orientation-change-event.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html

[Build Bot]

Created attachment 228249 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228239 [details]
initial version

Attachment 228239 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6046627127099392

New failing tests:
platform/mac/fast/scrolling/scroll-iframe-latched-mainframe.html
platform/mac/fast/scrolling/scroll-select-latched-mainframe.html
screen_orientation/screenorientation-api.html
screen_orientation/consecutive-locking.html
screen_orientation/orientation-change-event.html
platform/mac/fast/scrolling/scroll-div-latched-mainframe.html

[Build Bot]

Created attachment 228255 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Benjamin Poulain]

Comment on attachment 228239 [details]
initial version

View in context: https://bugs.webkit.org/attachment.cgi?id=228239&action=review

I only quickly skimmed over the patch, some comments bellow. This needs some serious cleanup.

> Source/WebCore/ChangeLog:7
> +

Here you should give: 
-An URL to the latest spec or draft (preferably the dated version).
-What is the status of support from other engines.
-Explain the architecture of your change.

> Source/WebCore/Modules/screenorientation/ScreenOrientationController.h:46
> +    void setClientForTest(ScreenOrientationClient*);
> +    bool hasClientForTest() const { return m_hasClientForTest; }

This is not a good way to test features like this.

You should have the mock object in the WebKit layer.

> Source/WebCore/bindings/js/JSScreenCustom.cpp:37
> +JSValue JSScreen::lockOrientation(ExecState* exec)

Why do you need a custom wrapper for this? If it is just for the promise, maybe we should extend the code generator?

> Source/WebCore/page/Screen.cpp:130
> +    static NeverDestroyed<AtomicString> portrait("portrait", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> portraitPrimary("portrait-primary", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> portraitSecondary("portrait-secondary", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> landscape("landscape", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> landscapePrimary("landscape-primary", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> landscapeSecondary("landscape-secondary", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> any("any", AtomicString::ConstructFromLiteral);

Why do you use AtomicStrings here?

> Source/WebCore/page/Screen.cpp:187
> +    for (unsigned i = 0; i < length; ++i) {
> +        if (map[i].name == orientationString) {

This is what compile time hash map are for.

> Source/WebCore/page/Screen.h:50
> +    enum ScreenOrientation {

If this is an exhaustive list, this should be a typed enum.

[Ryuan Choi]

(In reply to comment #13)
> (From update of attachment 228239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228239&action=review
> 
> I only quickly skimmed over the patch, some comments bellow. This needs some serious cleanup.
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> Here you should give: 
> -An URL to the latest spec or draft (preferably the dated version).
> -What is the status of support from other engines.
> -Explain the architecture of your change.
> 
First, thanks for the review.

I will revise the patch with your comments and latest spec/draft.

FYI, latest draft is below.
It was updated once more so patch is bit different with the below spec.

https://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html

> > Source/WebCore/Modules/screenorientation/ScreenOrientationController.h:46
> > +    void setClientForTest(ScreenOrientationClient*);
> > +    bool hasClientForTest() const { return m_hasClientForTest; }
> 
> This is not a good way to test features like this.
> 
> You should have the mock object in the WebKit layer.
OK, I will try.

> 
> > Source/WebCore/bindings/js/JSScreenCustom.cpp:37
> > +JSValue JSScreen::lockOrientation(ExecState* exec)
> 
> Why do you need a custom wrapper for this? If it is just for the promise, maybe we should extend the code generator?

I am not familiar with code generator but I will check.

> 
> > Source/WebCore/page/Screen.cpp:130
> > +    static NeverDestroyed<AtomicString> portrait("portrait", AtomicString::ConstructFromLiteral);
> > +    static NeverDestroyed<AtomicString> portraitPrimary("portrait-primary", AtomicString::ConstructFromLiteral);
> > +    static NeverDestroyed<AtomicString> portraitSecondary("portrait-secondary", AtomicString::ConstructFromLiteral);
> > +    static NeverDestroyed<AtomicString> landscape("landscape", AtomicString::ConstructFromLiteral);
> > +    static NeverDestroyed<AtomicString> landscapePrimary("landscape-primary", AtomicString::ConstructFromLiteral);
> > +    static NeverDestroyed<AtomicString> landscapeSecondary("landscape-secondary", AtomicString::ConstructFromLiteral);
> > +    static NeverDestroyed<AtomicString> any("any", AtomicString::ConstructFromLiteral);
> 
> Why do you use AtomicStrings here?
> 
> > Source/WebCore/page/Screen.cpp:187
> > +    for (unsigned i = 0; i < length; ++i) {
> > +        if (map[i].name == orientationString) {
> 
> This is what compile time hash map are for.
> 
I will change them to String and try to use hash map.

> > Source/WebCore/page/Screen.h:50
> > +    enum ScreenOrientation {
> 
> If this is an exhaustive list, this should be a typed enum.

I tried enum class but it requires operator&() .
If it is more preferred to add bit operator(), I will do it.

[Rodney Rehm]

Why hasn't there been any movement on this in 2 years?

The specification has moved - https://w3c.github.io/screen-orientation/ - it's still a Working Draft. An implementation landed in Blink in late 2015: https://bugs.chromium.org/p/chromium/issues/detail?id=162827

According to http://caniuse.com/#feat=screen-orientation and https://developer.mozilla.org/en-US/docs/Web/API/Screen/lockOrientation#Browser_compatibility this is even available in IE11, only WebKit is lacking support.

[Radar WebKit Bug Importer]

<rdar://problem/30427070>

[Simon Fraser (smfr)]

Please also add this to features.json (can be in a separate patch).

[Alexey Proskuryakov]

*** Bug 212931 has been marked as a duplicate of this bug. ***

[Blake Buell]

Any ETA? This would be extremely useful for standalone web apps.

[Jakub G (dailymotion)]

Note since this shipped with Chromium Edge, Safari is the last one remaining for compatibility (It even has partial support in IE11).

https://developer.mozilla.org/en-US/docs/Web/API/ScreenOrientation
https://caniuse.com/screen-orientation

[Sam Sneddon [:gsnedders]]

*** Bug 215124 has been marked as a duplicate of this bug. ***

[Ben Frain]

Just to add this is extremely frustrating to still, in 2022, need to add forks in the code for the older API purely for Safari.

Would love to see this in Safari. 

Could anyone on the WebKit team provide a rough ETA?

[Kim]

There should one unified API to query screen orientation.

https://developer.mozilla.org/en-US/docs/Web/API/Screen/orientation

As usual, (cough x2... hopefully Safari is not the new IE) Safari or webkit based browser is the only one not supporting this useful API.
https://caniuse.com/?search=screen.orientation



Using the following matchMedia to detect orientation poses a lot of bugs when attempt to support consistent experience across for both android and ios devices as android keyboard wasn't overlayed like ios device

> Note: This feature does not correspond to device orientation. Opening the soft keyboard on many devices in portrait orientation will cause the viewport to become wider than it is tall, thereby causing the browser to use landscape styles instead of portrait.


```
window.matchMedia("(orientation: portrait)");
```

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/orientation
https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Testing_media_queries

[Chris Dumez]

Resolved via subtasks.