Closed Bug 1754802 Opened 5 months ago Closed 4 months ago

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

Categories

(GeckoView :: General, defect, P1)

Unspecified
All

Tracking

(Webcompat Priority:P1, firefox98+ wontfix, firefox99+ fixed, firefox100 fixed)

RESOLVED FIXED
100 Branch
Webcompat Priority P1
Tracking Status
firefox98 + wontfix
firefox99 + fixed
firefox100 --- fixed

People

(Reporter: emilio, Assigned: m_kato)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file 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.

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.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

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.

Attachment #9263319 - Attachment is obsolete: true
Status: ASSIGNED → NEW
Depends on: 1754858
Severity: -- → S3
Priority: -- → P3

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.

Webcompat Priority: --- → ?

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

Webcompat Priority: ? → P1

Tracking for 98 as this is a P1 webcompat bug

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

Priority: P3 → P1
Flags: needinfo?(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.

Flags: needinfo?(m_kato)
Flags: needinfo?(emilio)
Flags: needinfo?(agi)
Blocks: 1755783

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

Flags: needinfo?(hikezoe.birchill)
Flags: needinfo?(botond)

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?

Flags: needinfo?(hikezoe.birchill)

(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.

Flags: needinfo?(agi)
No longer blocks: 1755783
Depends on: 1755783

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.

Flags: needinfo?(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.

Flags: needinfo?(agi)

(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.

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

Assignee: emilio → nobody

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.

Flags: needinfo?(agi)
Assignee: nobody → m_kato
Flags: needinfo?(botond)

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 [*].

See Also: → 1757031
Duplicate of this bug: 1756824
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/313c678374c9
Use WindowMetrics to get display size information on Android 12. r=geckoview-reviewers,agi
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch

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

Flags: needinfo?(m_kato)
Regressions: 1758548
No longer regressions: 1758548

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
Flags: needinfo?(m_kato)
Attachment #9264918 - Flags: approval-mozilla-beta?

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.

Attachment #9264918 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

You need to log in before you can comment on or make changes to this bug.