1754802 - Screen.width / Screen.height don't react to orientation in GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

See test-case. Screen.width / Screen.height are swapped on Chrome but not on GeckoView. If this worked, the calculations in the related bug wouldn't be wrong.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1754802 - Ensure safe-area calculations aren't too off. r=m_kato

The fix here is the EnsureAtMost call, but we can simplify a bit of the
related code by replacing the four conditionals with an
EnsureAtLeast call too, and using Deflate() rather than the manual
calculations.

This is sort of a wall paper over bug 1754323, but should be safe and
prevents ridiculously big safe areas.

Comment 2

[Phabricator Automation]

Comment on attachment 9263319 [details]
Bug 1754802 - Ensure safe-area calculations aren't too off. r=m_kato

Revision D138453 was moved to bug 1754323. Setting attachment 9263319 [details] to obsolete.

Comment 3

[Ksenia Berezina [:ksenia]]

Just came across an issue where Youtube in landscape mode is affected by this - as soon as you start watching a video, without going to fullscreen mode, the page becomes unscrollable and the bottom part of the video player can not be seen (i.e. video progress bar or the "Full-screen" button). They're relying on screen.height for calculations.

Comment 4

[Karl Dubost💡 :karlcow]

[Tracking Requested - why for this release]: This is affecting YouTube.
We are setting webcompat priority flag to P1 for the same reason.

Comment 5

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 98 as this is a P1 webcompat bug

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 7

[Emilio Cobos Álvarez (:emilio)]

So I was looking at this and I'm a bit baffled, because on a locally built GVE stuff just works. I had some cleanup patches but I confirmed they're not related to that.

Is there any chance that, for the same device, two different code-paths are being taken in this function?

If I understand correctly, getRealSize is orientation-aware (and what we want), while getWidth() and getHeight() are undocumented (and presumably are not). That could explain the behavior difference here... Agi, any thoughts?

I wonder if Fenix is somehow setting the screen size override, but I grepped on the android-components / fenix repos and found nothing.

Comment 8

[Karl Dubost💡 :karlcow]

just pinging Hiroyuki and Botond who might be interested too and/or have ideas about it.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

There seems to be nothing I know of. It sounds totally outside of Gecko. It looks like the getRealSize has been obsoleted, so that's a part of the problem?

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

So I was looking at this and I'm a bit baffled, because on a locally built GVE stuff just works. I had some cleanup patches but I confirmed they're not related to that.

Is there any chance that, for the same device, two different code-paths are being taken in this function?

If I understand correctly, getRealSize is orientation-aware (and what we want), while getWidth() and getHeight() are undocumented (and presumably are not). That could explain the behavior difference here... Agi, any thoughts?

I wonder if Fenix is somehow setting the screen size override, but I grepped on the android-components / fenix repos and found nothing.

I don't think Fenix sets a size override, and running this code under the debugger corroborates that.

I think this is some sort of timing issue, if I put a breakpoint in GeckoAppShell it works correctly, if I don't it doesn't update after rotation. I can look more tomorrow if needed.

Comment 11

[Makoto Kato [:m_kato]]

Hmm, my clean installed reference browser in my Pixel 4a (Android 12) cannot update screen info well on emilio's test case, but another OPPO device (Android 10) can update it correctly. Also I cannot reproduce this on emulator image of Android 12...

emilo, what your device and Android version?

getRealSize is deprecated in API from Android 12, so we may be able to resolve this by getCurrentWindowMetrics , but I don't know.

Comment 12

[Makoto Kato [:m_kato]]

agi, do you know anything that Pixel device doesn't return orientation-aware screen width and height by Display object? Even if using WindowMetrics, it returns same result.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Makoto Kato [:m_kato] from comment #11)

Hmm, my clean installed reference browser in my Pixel 4a (Android 12) cannot update screen info well on emilio's test case, but another OPPO device (Android 10) can update it correctly. Also I cannot reproduce this on emulator image of Android 12...

FWIW I'm using https://crisal.io/tmp/screen-orientation-change.html as a test-case too to see the timing of resizes and orientation changes and so on.

emilo, what your device and Android version?

My device is a OnePlus 9 w/ Android 12.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Unassigning for now as we figure out what the right fix here is.

Comment 15

[Makoto Kato [:m_kato]]

Ah, I guess we have to use createWindowContext for Display if Android 12. GeckoAppShell.getApplicationContext may not be better for this usages on Android 12.

Comment 16

[Makoto Kato [:m_kato]]

WIP: https://treeherder.mozilla.org/jobs?repo=try&revision=f761072a5eeaac3e7331460416d642b74db150c1

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Bug 1754802 - Use WindowMetrics to get display size information on Android 12. r=#geckoview-reviewers

When using Android 12, Display.getRealSize() may not return orientation-aware
width/height. Actually although we use application context to get display
manager and window manager for screen size information, we should create window
context if Android 12+, then use new WindowMetrics API [*].

• https://developer.android.com/about/versions/12/deprecations#display-api-deprecations

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/313c678374c9
Use WindowMetrics to get display size information on Android 12. r=geckoview-reviewers,agi

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/313c678374c9

Comment 21

[Donal Meehan [:dmeehan]]

:m_kato Since it's a Webcompat P1 looks like this would be a candidate for a beta uplift request?

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 9264918 [details]
Bug 1754802 - Use WindowMetrics to get display size information on Android 12. r=#geckoview-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: When user uses Android 12, screen object doesn't return correct width and height after changing orientation
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change is only for Android 12+ and this fix uses new API that is recommend by Google.
• String changes made/needed: N/A

Comment 23

[Donal Meehan [:dmeehan]]

Comment on attachment 9264918 [details]
Bug 1754802 - Use WindowMetrics to get display size information on Android 12. r=#geckoview-reviewers

Approved for 99.0b3. Thanks.

Comment 24

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a92b9c3dffcc

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Not a good candidate for a dot release IMO. Feel free to NI if you strongly disagree.