Closed Bug 1053975 Opened 5 years ago Closed 5 years ago

Trivial fullscreen app gets incorrect viewport on Flame (contentScale=1.5)

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: BenWa, Assigned: botond)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file app.zip
+++ This bug was initially created as a clone of Bug #1052751 +++

The attached app is a simple body { background: red; } with no meta viewport tag.

I believe the app should either be width 918 or have the width of the screen. However I'm seeing that the viewport is, when converted to device pixels, off by 1.5x.

I/Gecko   ( 1198):         RefLayerComposite (0xb0107400) [shadow-clip=(x=0, y=0, w=480, h=854)] [shadow-visible=< (x=0, y=0, w=480, h=854); >] [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [id=4]
I/Gecko   ( 1198):           ContainerLayerComposite (0xabdbfc00) [shadow-transform=[ 0.444444 0; 0 0.444444; 0 0; ]] [shadow-visible=< (x=0, y=0, w=480, h=854); >] [postScale=1.5, 1.5] [transform=[ 0.666667 0; 0 0.666667; 0 0; ]] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) cb=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) critdp=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) scrollableRect=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) scrollId=3 }] [preScale=1.5, 1.5]

The viewport is w=480 but it should be 480/1.5 since it's in CSS Pixels. I wonder if we don't take the content scale into account when we hit this patch.
Blocks: 1052751
No longer depends on: 1052751
First, let's clarify what viewport width we expect: we expect 320 (480 / 1.5), not 980, because in bug 940036, we added special handling for apps that fail to specify a meta viewport tag, treating them as if they had "width=device-width, height=device-height, user-scalable=no".

Now, let's see why we're getting 480 instead of 320:

  - nsDocument::GetViewportInfo() constructs the nsViewportInfo object
    for an app with no meta viewport tag here [1], passing in the
    screen size as an ScreenSize, which (correctly) has width=480

  - nsViewportInfo stores the size as a CSSSize. To convert from the
    ScreenSize to the CSSSize, it divides by 'mDefaultZoom', which is
    a CSSToScreenScale, here [2].

  - CSSToScreenScale is supposed to include the device scale, which is
    1.5 on a Flame.

  - However, the nsViewportInfo constructor used at [1] initializes
    mDefaultZoom to 1.0. As a result, the CSSSize that is calculated
    and stored has width=480 (incorrectly).

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7537
[2] http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsViewportInfo.h#35
Attached patch Fix (obsolete) — Splinter Review
Try push that includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=0eed380c2ead
Assignee: nobody → botond
Attachment #8473226 - Flags: review?(bugmail.mozilla)
The fix seems sound, but can you rebase this patch on top of bug 974242? mbrubeck should review this as well.
(In reply to Botond Ballo [:botond] from comment #2)
> Try push that includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=0eed380c2ead

The build failures are not related to this patch :) (That's what I get for trying to combine Try pushes.)
Comment on attachment 8473226 [details] [diff] [review]
Fix

Review of attachment 8473226 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review for now; I can review the rebased version.
Attachment #8473226 - Flags: review?(bugmail.mozilla)
Attached patch FixSplinter Review
Attachment #8473226 - Attachment is obsolete: true
Attachment #8473733 - Flags: review?(mbrubeck)
Attachment #8473733 - Flags: review?(bugmail.mozilla)
Rebased as requested. Marking bug as dependent on bug 974242 accordingly.
Depends on: 974242
Attachment #8473733 - Flags: review?(bugmail.mozilla) → review+
Attachment #8473733 - Flags: review?(mbrubeck) → review+
Trying again now that bug 974242 has landed: https://tbpl.mozilla.org/?tree=Try&rev=99c64793844d
https://hg.mozilla.org/mozilla-central/rev/68d28e7fb162
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.