940036 - 3rd party gaia apps should set a meta-viewport tag

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

+++ This bug was initially created as a clone of Bug #916185 +++

If we want to turn on APZC for all gaia apps, the default meta-viewport behaviour is going to change. We should reach out to 3rd party app developers and ensure they always specify a meta-viewport tag in their apps, or the behaviour might change out from under them. Bug 916185 covers updating our own gaia apps to do the same thing.

Comment 1

[Mark Coggins]

I've added Bill Walker and Karen Ward to the cc list. Bill is responsible for partner engineering (who works with all the big 3rd party app developers) and Karen is the program manager for Marketplace content. I work with the indie developers, so probably between the three of us we can get things rolling. I still have questions about the implications, but will huddle with them as a first step.

Comment 2

[:Harald Kirschner :digitarald]

Mobile apps set it by design as CSS depends on it and mobile experience is broken when allowing zooming.

To confirm we might want to run an automated script through the Marketplace that checks if the URL at launch_path has a viewport meta tag? That would be good to have in place for other future occasions where we need a ecosystem-wide sanity check.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Kats, correct me if I'm wrong, but the apps that want to retain the current default behaviour would just need to add this:
 <meta name="viewport" content="width=device-width; initial-scale=1; user-scalable=no">

Comment 4

[Kartikaya Gupta (email:[e-mail])]

That's correct.

Comment 5

[Benoit Girard (:BenWa)]

Why can't the APZC work as-is pre-APZC if they don't set a meta tag. i.e. default to what milan said if we don't have a meta tag? I noticed forcing APZC on Settings app changes the behavior quite a bit.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

My understanding is that we need to change the behaviour of "the meta-viewport tag is not specified".  If any of the apps explicitly set the tag, we're fine - it's just the ones that don't set it at all that are in trouble, as we picked the "wrong" default, inconsistent with the specs, Fennect, etc.

Comment 7

[Benoit Girard (:BenWa)]

I'd like to know what the change is. The state of APZC is an implementation details and should be transparent to the user (other then improved performance). So I imagine that Settings app should behavor the same way if APZC is on or off when there is no meta tag. My question is why isn't that so?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

So without APZC the default meta-viewport behaviour is wrong. I mentioned at [1] that I don't know how this happened. I suspect primarily it's because pre-APZC there was no code to do pinch-zoom, and so if things got rendered the way they should (with a desktop-sized CSS viewport) then there would be no way to zoom them in to a usable size. With APZC enabled we run into different code paths in TabChild.cpp that actually read and respect the meta-viewport tag, and so the behaviour changes.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=916185#c4

Comment 9

[Benoit Girard (:BenWa)]

And preserving our current behavior would make us non spec-compliant?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I don't think there's any actual spec for this but it would be inconsistent with our other platforms.

Comment 11

[Florian Bender]

(In reply to (PTO Nov21-Dec01) Kartikaya Gupta (email:[e-mail]) from comment #10)
> I don't think there's any actual spec for this but it would be inconsistent
> with our other platforms.

There is a spec: http://dev.w3.org/csswg/css-device-adapt/ (Bug 747754)

… and I'd rather see that implemented properly than fiddling with the meta-viewport again.

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to Benoit Girard (:BenWa) from comment #9)
> And preserving our current behavior would make us non spec-compliant?

Yes, and it would also mean that exact same code renders and behaves differently in the browser than it does in an installed app.  I think we want web apps to render and behave the same whether they are running in the browser or as installed apps.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug940036.patch

While I agree on the theory (aka we should just do nothing to be right), we can not just break user experience for applications that are installed on phones in the wild.
The core of this issue was our bad, and while it is painful to have to violate the spec for a little while it won't hurt more than the current situation.

So I'm in favor of landing a workaround for a smoother transition. That will offer some time for our app developer to update their apps, and it will does not degrade the user experience for phone in the wild. We need a way to gather data from the marketplace to know when apps are ready. While it is done we can then wait a little bit to make sure our users have updated their apps, and just remove the workaround.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8341054 [details] [diff] [review]
bug940036.patch

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

::: content/base/public/nsViewportInfo.h
@@ +40,5 @@
> +    nsViewportInfo(const mozilla::ScreenIntSize& aDisplaySize,
> +                   bool aAllowZoom) :
> +      mDefaultZoom(1.0),
> +      mAutoSize(true),
> +      mAllowZoom(aAllowZoom)

Instead of adding a new constructor, can we just modify the existing one above to also take aAllowZoom? We can set the default value for aAllowZoom to true, or just modify the call sites (there aren't that many).

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Comment on attachment 8341054 [details] [diff] [review]
> bug940036.patch
> 
> Review of attachment 8341054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/public/nsViewportInfo.h
> @@ +40,5 @@
> > +    nsViewportInfo(const mozilla::ScreenIntSize& aDisplaySize,
> > +                   bool aAllowZoom) :
> > +      mDefaultZoom(1.0),
> > +      mAutoSize(true),
> > +      mAllowZoom(aAllowZoom)
> 
> Instead of adding a new constructor, can we just modify the existing one
> above to also take aAllowZoom? We can set the default value for aAllowZoom
> to true, or just modify the call sites (there aren't that many).

Makes sense.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug940036.patch v2

Use |true| as a default value.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33904f2323d

I opened bug 946088 as a followup to evangelize the meta viewport tag to marketplace developers and once it is done this fix can be safely removed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f33904f2323d

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Coggins from comment #1)
> I've added Bill Walker and Karen Ward to the cc list. Bill is responsible
> for partner engineering (who works with all the big 3rd party app
> developers) and Karen is the program manager for Marketplace content. I work
> with the indie developers, so probably between the three of us we can get
> things rolling. I still have questions about the implications, but will
> huddle with them as a first step.

I was just wondering if you still have questions or if you have everything you need to reach out to developers. I notice that bug 946088 is currently unowned and I'm not sure if anything has been done there yet. The APZ code has been turned on for Gaia apps by default for b2g-1.3+, and so I want to make sure that we can remove this workaround in a release or two.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Bill, Karen - either of you know the status either? (re: comment 1 and comment 19)

Comment 21

[Mark Coggins]

Kartikaya,

Sorry for the tardy response. We didn't move forward with any action when the tie to a specific release went away.

I'll discuss with Bill and Karen in our upcoming content meeting. It seems like a check in the app validation script that's run on submission might be the first thing to do to avoid making the problem worse. (I'm assuming that's possible.) Then what Harald suggested about doing a script to run over existing content and identify apps with issues might be better than doing a blanket mailing to everyone.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Coggins from comment #21)
> Sorry for the tardy response. We didn't move forward with any action when
> the tie to a specific release went away.

As far as I know the plan is to keep the workaround for a couple of releases (1.3 and 1.4) and then remove it (in 1.5). I don't believe that's set in stone but I'd like to adhere to that as much as possible.

> I'll discuss with Bill and Karen in our upcoming content meeting. It seems
> like a check in the app validation script that's run on submission might be
> the first thing to do to avoid making the problem worse. (I'm assuming
> that's possible.) Then what Harald suggested about doing a script to run
> over existing content and identify apps with issues might be better than
> doing a blanket mailing to everyone.

Thanks. Please update bug 946088 (or this one) with status updates as you have them.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Hi Mark, just to let you know that we are taking out this code in B2G 2.1 (as part of bug 1084321). Any affected apps that have not yet updated will likely be broken in that release.

Comment 24

[Mark Coggins]

I'm leaving Mozilla at the end of the month, but I've shared the status with the right folks in the Marketplace team.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Sorry to hear you're leaving! Can you let me know who on the marketplace team I can follow-up with in case I need to?

Comment 26

[Mark Coggins]

I would start with Bill Walker (who is on the bug, along with Harald Kirschner, who is in his group) as well as Lisa Brewster. I've notified Bill and Lisa separately by email.

Comment 27

[:Harald Kirschner :digitarald]

We haven't seen any apps break so far and would take this on per-app basis for partners. The review team should be on the lookout though to be able to identify the issue correctly when it pops up as soon as reviewers start testing on 2.1.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mark Coggins from comment #26)
> I would start with Bill Walker (who is on the bug, along with Harald
> Kirschner, who is in his group) as well as Lisa Brewster. I've notified Bill
> and Lisa separately by email.

Awesome, thanks! I've CC'd them on bug 946088 which was filed to track that work.

(In reply to Harald Kirschner :digitarald from comment #27)
> We haven't seen any apps break so far and would take this on per-app basis
> for partners. The review team should be on the lookout though to be able to
> identify the issue correctly when it pops up as soon as reviewers start
> testing on 2.1.

Cool. We just landed the code on inbound and will probably uplift it to 2.1 in the next few days. So if apps mysteriously start rendering zoomed out or becoming zoomable after that point this is likely why.