Closed Bug 909887 Opened 6 years ago Closed 6 years ago

Homescreen renders at 1.5 scale when APZC is turned on

Categories

(Firefox OS Graveyard :: Gaia, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files)

When the patch from bug 909877 is applied to turn on APZC everywhere, the homescreen ends up rendering at 1.5 scale on my geeksphone peak. Because of bug 909881 i can pinch-zoom out to make it look normal (which also improves the behaviour in general) but in general this is not a desirable state of affairs.

I think that the homescreen rendering at 1.5 scale is actually "correct" because the meta-viewport tag in the homescreen app says "width=device-width,user-scalable=no,initial-scale=1.0" and the device is a hi-dpi device which renders everything at 1.5x. Based on a chat with wesj I believe there is some code somewhere that forces the scale to be 1.0 currently and somehow turning on APZC bypasses that code. I need to find it and add to the APZC codepath. Or we could fix all the apps to not force an initial-scale=1.0 and do something more correct here.
Kats, can you email dev-gaia to explain how we should fix the meta viewports in apps? Let's do the correct thing...
I was just discussing this with mbrubeck and currently there is actually no way for content authors to force the value of the devicePixelRatio. So in this case there is actually nothing the apps themselves can do, and if we take out all our special-case behaviour code, apps are going to render at 1.5x on the peak. There was a proposal for a target-densitydpi meta viewport property (bug 737090) but we decided not to implement that. Bug 677989 is also on file for some way to make this happen.
Depends on: 677989
Attached image Screenshot
This is what I see, for the record.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Created attachment 799518 [details]
> Screenshot
> 
> This is what I see, for the record.

Let's pretend this is a special accessibility mode ;)
I agree with comment 0 that rendering apps and content at 150% on an "HDPI" display is a feature, not a bug.  It means that apps will remain an appropriate physical size as display density increases.  This is exactly what happens to web apps when they are running on a high-density display on Android, Chrome OS, Windows, iOS, or Mac OS X.   Having B2G behave differently was a bug.

The normal way to avoid blurriness is to still use initial-scale=1.0, but provide images with an appropriate resolution and lay them out at an appropriate size (either one image with the maximum resolution you want to support, or a choice of images via media queries, srcset, etc.).

It looks like the Gaia homescreen app already has code to support high-resolution images, but it looks like it is choosing the wrong images for some reason.  Note the comment here:

>  // Be aware that the current manifest icon description syntax does
>  // not distinguish between 60@1.5x and 90@1x, so we would have to use
>  // the latter as the former.
https://github.com/mozilla-b2g/gaia/blob/v1.1.0hd/apps/homescreen/js/grid.js#L6

The dialer manifest contains:

>      "icons": {
>        "120": "/dialer/style/icons/Dialer.png",
>        "60": "/dialer/style/icons/60/Dialer.png"
>      },
https://github.com/mozilla-b2g/gaia/blob/v1.1.0hd/apps/communications/manifest.webapp#L14

but both of those paths actually lead to 60x60 PNG files.  There is no 120x120 icon as the manifest claims.  With a devicePixelRatio of 1.5, bestMatchingIcon will end up choosing the lying "120" entry:
https://github.com/mozilla-b2g/gaia/blob/v1.1.0hd/apps/homescreen/js/grid.js#L1067

...when what we really want is the 90x90 icon, /dialer/style/icons/60/Dialer@1.5x.png.  So I suspect the blurry icons could be fixed by changing the app manifests.
For comparison. Ignore the green/purple borders, I had layer borders turned on.
Note that the screenshots are not full-resolution; they are scaled by 2/3 (to 360x640, from the native Peak resolution of 540x960).  That confused me; I thought that the icons were blurry but the correct size (60 CSS px == 90 device pixels).  Actually they are 50% too large (90 CSS px == 120 device pixels).  So this isn't just a matter of selecting the correct assets; somewhere the layout itself is going wrong.
Milan, can you find an owner here? Brad is trying to carry APZC over the finish line for 1.2. This blocks that and if that succeeds, it might create an additional avenue for us to address some of the performance problems we are facing.
Flags: needinfo?(milan)
For anyone newly looking at this bug, please note that my speculation about icon path selection bugs in comment 5 was wrong and should be ignored; it was based on misinterpreting the screenshot sizes as noted in comment 7.

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Actually they are 50% too large (90 CSS px == 120 device pixels).

And this contained a simple arithmetic error; it should read:
"Actually they are 50% too large (90 CSS px == 135 device pixels)."

The actual bug is apparently that we are applying 1.5x scaling twice, to end up with a 2.25 dppx ratio (1.5 * 1.5).
Moved some bugs around, Kats should be able to look at this later today or tomorrow.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(milan)
This bug looks different now (presumably affected by the patches from bug 912806 that landed yesterday). Now when I try to repro the icons in the bottom dock are offscreen, which indicates that it's not just a layout viewport size problem any more (although that might still be happening) but there's also an async zoom problem. I will look into both I guess.
So the main problem here is that mLastMetrics.mDevPixelsPerCSSPixel is not initialized properly in TabChild. Initializing it in the before-first-paint handler makes things significantly better. There's still a bit of a problem with the background image not painting unless you zoom in the homescreen a tiny bit but I will continue investigating.
Attached patch PatchSplinter Review
With recent m-c code this fixes the initial zoom. The patch on bug 909881 (which may need some cleanup) prevents the user from being able to zoom the homescreen.

However there's still the weird issue of the background not painting initially. If you pan the homescreen the background appears while panning but then disappears again. If you don't above the patch from bug 909881 then zooming the homescreen seems to permanently fix the background, even if you zoom back out to the original zoom level.

Brad was saying that he saw a similar background-not-painting issue with the tiling patches applied so I suspect that it's somewhere deep in gfx code.
I split the background painting issue to bug 915872.
Comment on attachment 803934 [details] [diff] [review]
Patch

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

::: dom/ipc/TabChild.cpp
@@ +424,5 @@
>          // the top-left of the page.
>          mLastMetrics.mViewport = CSSRect(CSSPoint(), kDefaultViewportSize);
>          mLastMetrics.mCompositionBounds = ScreenIntRect(ScreenIntPoint(), mInnerSize);
>          mLastMetrics.mZoom = mLastMetrics.CalculateIntrinsicScale();
> +        mLastMetrics.mDevPixelsPerCSSPixel = CSSToLayoutDeviceScale(mWidget->GetDefaultScale());

Is there any possibility of having a full zoom at this stage?
I don't believe so; this only runs when a child process is first created and so it hasn't had a chance to acquire a full zoom yet. I think.
Comment on attachment 803934 [details] [diff] [review]
Patch

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

Ok, looks reasonable then.
Attachment #803934 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/b56ae6533034
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.