Closed Bug 1053975 Opened 10 years ago Closed 10 years ago

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


(Core :: Graphics: Layers, defect)

Not set





(Reporter: BenWa, Assigned: botond)




(2 files, 1 obsolete file)

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

Attached patch Fix (obsolete) — Splinter Review
Try push that includes this patch:
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:

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]

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:
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.