Adding screen orientation
Has this feature been announced on webkit-dev? I couldn't quickly find such an e-mail.
(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.
Created attachment 228239 [details] initial version
*** Bug 125914 has been marked as a duplicate of this bug. ***
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
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
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
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
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
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
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
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
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.
(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.
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.
<rdar://problem/30427070>
Please also add this to features.json (can be in a separate patch).
*** Bug 212931 has been marked as a duplicate of this bug. ***
Any ETA? This would be extremely useful for standalone web apps.
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
*** Bug 215124 has been marked as a duplicate of this bug. ***
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?
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
Resolved via subtasks.