1084321 - Pages navigated to by following links from Facebook app on B2G cannot be zoomed

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Chris Lord [:cwiiis]]

[Blocking Requested - why for this release]: Lots of desktop sites are basically unusable

I come across a lot of desktop pages on b2g that can't be zoomed in the system browser for no apparent reason (they have no meta viewport and they scroll on both axes). I've put such an example in the URL.

Such pages are basically unreadable. I believe this is a regression, but I've no idea when exactly this happened (as, thankfully, most sites are responsive/have mobile versions).

This affects 2.1 and I think is a serious enough UX regression to block on. I don't believe this affects fennec, but I'll test this later.

Comment 1

[Chris Lord [:cwiiis]]

Can confirm this doesn't affect fennec beta at least.

Comment 2

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

On a Flame running relatively recent code (a few days ago) I can zoom just fine on the URL provided. Botond, are you able to reproduce this?

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> On a Flame running relatively recent code (a few days ago) I can zoom just
> fine on the URL provided. Botond, are you able to reproduce this?

This is odd, I flashed 2.1 to confirm and reproduced... Maybe it was my method of going to the link (via a Facebook link) that causes the issue...? Pasting for reference, will try to confirm as well. https://www.facebook.com/l.php?u=https%3A%2F%2Fwww.techdirt.com%2Farticles%2F20141015%2F13561128840%2Ffirst-commerce-department-fines-intel-subsidiary-exporting-encryption.shtml&h=cAQFzJjuDAQEQjQkekcFUEpVT_7YwO1_OdleQEECBc1Q3Og&enc=AZNwfT_eaehWR4st2xJJoieXoBMZl9OoNF65wFzj_D34xqZqQbO0m-uyXfbeySIUQ95kT45V_dOVjKhaM_3MdvOODYZfSkq-mAEXEp1p3UWBQvY6_IMaMgZHAV7E_eJF5qfrQN9LqOnwInAdnXgrkbSb&s=1

Comment 4

[Chris Lord [:cwiiis]]

Following either link from this bug does not reproduce the error, so perhaps it's only windows opened via Facebook that exhibit it?

Comment 5

[Chris Lord [:cwiiis]]

Confirmed.

STR:

1. Log into facebook via the Facebook app from the marketplace (not tested via website)
2. Follow a link to a site that would ordinarily be zoomable

Expected:

Page opens, can be zoomed

Actual:

Page opens, cannot be zoomed

Comment 6

[Chris Lord [:cwiiis]]

This does not occur if you visit facebook via the website rather than the app (which is exactly the same, just packaged differently).

I expect this applies to all links followed from apps that open in an external window, but I'll leave the bug to reference what I've verified.

Comment 7

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

Do you know what code is invoked when apps open links? Presumably there's a window.open call somewhere in Gaia that is spawning the new window.

Comment 8

[Chris Lord [:cwiiis]]

I assume this file handles the request? https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/child_window_factory.js

Comment 9

[Chris Lord [:cwiiis]]

So the event gets sent here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/child_window_factory.js#L147

and handled here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/wrapper_factory.js#L15
which looks like it creates the AppWindow here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window_factory.js#L194

this ends up in this block: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L544

So it uses the iframe from the 'frameElement' detail on the original mozbrowseropenwindow event... Not sure where this comes from just yet.

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #9)
> So the event gets sent here:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> child_window_factory.js#L147
> 
> and handled here:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> wrapper_factory.js#L15

Ordering slightly wrong here, child_window_factory handles it, not wrapper_factory (which only handles window.open with remote=true). I guess the problem here is the resulting iframe isn't the root frame, so doesn't get zooming(?)

Unfortunate, but if that's the case, I suppose this can't really be a blocker. Is there a reason why _blank target pages don't get remote frames? Visually, they're handled exactly the same now (vs. how it was pre-2.1(/2.0?)).

Comment 11

[Chris Lord [:cwiiis]]

I'm guessing this chain of events is initiated Gecko-side by OpenWindowInProcess http://mxr.mozilla.org/mozilla-central/ident?i=OpenWindowInProcess (as opposed to OpenWindowOOP http://mxr.mozilla.org/mozilla-central/ident?i=OpenWindowOOP)

Comment 12

[Chris Lord [:cwiiis]]

I guess this is also the reason why navigating to YouTube links from Facebook doesn't let you fullscreen videos(?)

Comment 13

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding qawanted to reproduce this and to check 2.0 for a possible regression window.

Comment 14

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue DOES occur on Flame KK 2.2 and Flame KK 2.1 shallow flash.

Environmental Variables:
Device: Flame 2.2
BuildID: 20141020055012
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: f2d7d694aae5
Version: 36.0a1 (2.2) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20141020105810
Gaia: dc26d4a1c28682137130c62058399b43a4ea86b4
Gecko: 7e96ff7c7a64
Version: 34.0 (2.1) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

This issue does NOT occur on Flame KK 2.0 shallow flash after 10 attempts.

Results: 
Environmental Variables:
Device: Flame 2.0
BuildID: 20141020025114
Gaia: 63b56a7a7453726b9e12ad1afe02c68c83c5aeca
Gecko: 09b9387be5ad
Version: 32.0 (2.0) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 15

[Candice Serran (:cserran)]

definite broken functionality.

Comment 16

[Chris Lord [:cwiiis]]

Before I dig further into this, do you have any comment on this Alive? (knowing the window management code a lot better than I do)

Comment 17

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Bug 1060191 seems to be the cause of this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.2
BuildID: 20140926001739
Gaia: 40dff263f02982b47fe548c403cd5b57d69b8d18
Gecko: 41ae5078b2ff
Version: 35.0a1 (2.2) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

First Broken 
Environmental Variables:
Device: Flame 2.2
BuildID: 20140926003241
Gaia: 6163c5ee0f99f73439318d064e0862ccd4e11a49
Gecko: ce0b48c0f6d1
Version: 35.0a1 (2.2) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 40dff263f02982b47fe548c403cd5b57d69b8d18
Gecko: ce0b48c0f6d1

First Broken gaia / Last Working gekko - Issue DOES occur
Gaia: 6163c5ee0f99f73439318d064e0862ccd4e11a49
Gecko: 41ae5078b2ff

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/40dff263f02982b47fe548c403cd5b57d69b8d18...6163c5ee0f99f73439318d064e0862ccd4e11a49

Comment 18

[Joshua Mitchell (Inactive)]

Broken by Bug 1060191 

Chris - since you are the 'Assigned To' on this bug I'm NI'ing you instead of the Patch Author

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Before I dig further into this, do you have any comment on this Alive?
> (knowing the window management code a lot better than I do)

I guess we are not doing something for APZ enabling for the mozbrowser iframe provided by mozbrowseropenwindow event detail.iframe coming from gecko?

What bug 1060191 did is, we were always creating a new mozbrowser iframe in gaia side for every mozbrowseropenwindow event in the past, but now we tend to re-use the iframe given by gecko.

Comment 20

[Chris Lord [:cwiiis]]

I'm not convinced that this is a Gaia bug at all - bug 1060191 changes things so that rather than Gaia, incorrectly, creating a new window for every window.open, it reuses the iframe that comes from event detail, which I presume is created by Gecko.

So this iframe that we get from platform doesn't seem to support zooming for whatever reason (my first assumption would be that it's not the root frame) and I'm not sure there's anything we can do about that? (unless we want to break spec and revert bug 1060191)

I'll look into this and confirm/obsolete these assumptions.

Comment 21

[Chris Lord [:cwiiis]]

Well, this is puzzling - it seems not all windows opened from Facebook aren't zoomable, only some... All the opened windows are remote, so they ought to be zoomable - so this could well be a Gaia problem I suppose, but it's weird that some are zoomable and some aren't... I've added some logging in various bits of Gaia and Gecko.

Here's launching a zoomable link:

I/Gecko   ( 5063): XXX: OpenWindowOOP
I/GeckoConsole( 5063): Content JS LOG: XXX Opening window: {"features":"","frameElement":{},"name":"_blank","url":"http://lm.facebook.com/l.php?u=http%3A%2F%2Ft.co%2FYlEvX0Buvb&h=dAQE73K7f&enc=AZOw-41Ac9wSL0JjIeJaTe3gsfOltaeaw8X6pt5E1iSWX8UyQWZWho-L5dCetTlMWNgExtX0GYr8MCuL8Hf903QYh6BsSpgHgv1kTNvYwRHUsm04hTF2W-qu9dNXlUxUfu753kdafcocP8wp88dkVLsD3b3Wxoa0pfbzjOEUKYqzrQ&s=1"} 
I/GeckoConsole( 5063): Content JS LOG: XXX Ignoring, not remote 
I/GeckoConsole( 5063): Content JS LOG: XXX Creating new window: {"url":"http://lm.facebook.com/l.php?u=http%3A%2F%2Ft.co%2FYlEvX0Buvb&h=dAQE73K7f&enc=AZOw-41Ac9wSL0JjIeJaTe3gsfOltaeaw8X6pt5E1iSWX8UyQWZWho-L5dCetTlMWNgExtX0GYr8MCuL8Hf903QYh6BsSpgHgv1kTNvYwRHUsm04hTF2W-qu9dNXlUxUfu753kdafcocP8wp88dkVLsD3b3Wxoa0pfbzjOEUKYqzrQ&s=1","name":"_blank","iframe":{}} 
I/GeckoConsole( 5063): Content JS LOG: XXX Creating AppWindow: {"url":"http://lm.facebook.com/l.php?u=http%3A%2F%2Ft.co%2FYlEvX0Buvb&h=dAQE73K7f&enc=AZOw-41Ac9wSL0JjIeJaTe3gsfOltaeaw8X6pt5E1iSWX8UyQWZWho-L5dCetTlMWNgExtX0GYr8MCuL8Hf903QYh6BsSpgHgv1kTNvYwRHUsm04hTF2W-qu9dNXlUxUfu753kdafcocP8wp88dkVLsD3b3Wxoa0pfbzjOEUKYqzrQ&s=1","iframe":{},"name":"_blank","origin":"http://lm.facebook.com/l.php?u=http%3A%2F%2Ft.co%2FYlEvX0Buvb&h=dAQE73K7f&enc=AZOw-41Ac9wSL0JjIeJaTe3gsfOltaeaw8X6pt5E1iSWX8UyQWZWho-L5dCetTlMWNgExtX0GYr8MCuL8Hf903QYh6BsSpgHgv1kTNvYwRHUsm04hTF2W-qu9dNXlUxUfu753kdafcocP8wp88dkVLsD3b3Wxoa0pfbzjOEUKYqzrQ&s=1","manifestURL":"","manifest":null,"evtType":"openwindow"} 
I/GeckoConsole( 5063): Content JS LOG: XXX We have an iframe 

Here's launching an unzoomable link:

I/Gecko   ( 5063): XXX: OpenWindowOOP
I/GeckoConsole( 5063): Content JS LOG: XXX Opening window: {"features":"","frameElement":{},"name":"_blank","url":"https://m.facebook.com/l.php?u=https%3A%2F%2Fwww.techdirt.com%2Farticles%2F20141015%2F13561128840%2Ffirst-commerce-department-fines-intel-subsidiary-exporting-encryption.shtml&h=RAQFyr2t6&enc=AZMA4lw7C8TYB9cwVGIHewnFZMFuYvyp9lWLzo0bgrwuBNJ4xK_4o5YdHeQkuG93MWYrKp2PK4SygC9ldzCtCnGf&s=1"} 
I/GeckoConsole( 5063): Content JS LOG: XXX Ignoring, not remote 
I/GeckoConsole( 5063): Content JS LOG: XXX Creating new window: {"url":"https://m.facebook.com/l.php?u=https%3A%2F%2Fwww.techdirt.com%2Farticles%2F20141015%2F13561128840%2Ffirst-commerce-department-fines-intel-subsidiary-exporting-encryption.shtml&h=RAQFyr2t6&enc=AZMA4lw7C8TYB9cwVGIHewnFZMFuYvyp9lWLzo0bgrwuBNJ4xK_4o5YdHeQkuG93MWYrKp2PK4SygC9ldzCtCnGf&s=1","name":"_blank","iframe":{}} 
I/GeckoConsole( 5063): Content JS LOG: XXX Creating AppWindow: {"url":"https://m.facebook.com/l.php?u=https%3A%2F%2Fwww.techdirt.com%2Farticles%2F20141015%2F13561128840%2Ffirst-commerce-department-fines-intel-subsidiary-exporting-encryption.shtml&h=RAQFyr2t6&enc=AZMA4lw7C8TYB9cwVGIHewnFZMFuYvyp9lWLzo0bgrwuBNJ4xK_4o5YdHeQkuG93MWYrKp2PK4SygC9ldzCtCnGf&s=1","iframe":{},"name":"_blank","origin":"https://m.facebook.com/l.php?u=https%3A%2F%2Fwww.techdirt.com%2Farticles%2F20141015%2F13561128840%2Ffirst-commerce-department-fines-intel-subsidiary-exporting-encryption.shtml&h=RAQFyr2t6&enc=AZMA4lw7C8TYB9cwVGIHewnFZMFuYvyp9lWLzo0bgrwuBNJ4xK_4o5YdHeQkuG93MWYrKp2PK4SygC9ldzCtCnGf&s=1","manifestURL":"","manifest":null,"evtType":"openwindow"} 
I/GeckoConsole( 5063): Content JS LOG: XXX We have an iframe 

Each case seems to be handled identically, so I assume that something in the actual content of the page is triggering this behaviour(?)

Comment 22

[Chris Lord [:cwiiis]]

So, interestingly, the first link has a meta viewport and the second doesn't - could be we're not handling opened windows without an explicit viewport correctly?

Comment 23

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

Is it hitting http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7673 ?

Comment 24

[Chris Lord [:cwiiis]]

For this second page, Gecko is under the impression that it has a viewport when it doesn't - putting debug output in content/base/src/nsDocument.cpp nsDocument::GetViewportInfo, you can see that it takes the DisplayWidthHeightNoZoom case.

I'm not sure where it's getting this from, my assumption would be that it gets it from the Mobile facebook link redirector and the document gets reused on redirection, whereby the viewport isn't reset to Unknown and it carries on getting used (unless the page specifies another meta viewport).

This appears to be a platform problem, changing component to reflect.

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Is it hitting
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> cpp#7673 ?

This does get hit at some point, but given that the page isn't zoomable, I'm assuming that it must be for some other frame... Need to add some more detailed logging.

Comment 26

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

(In reply to Chris Lord [:cwiiis] from comment #25)
> This does get hit at some point, but given that the page isn't zoomable, I'm
> assuming that it must be for some other frame... Need to add some more
> detailed logging.

If it's hitting that that means the page is being treated as an "app" rather than a "browser", and it has no meta-viewport tag, so we give it a default meta-viewport tag that is not zoomable [1]. This was done for backwards compatibility. Gaia should not be opening it as an "app", it should be opening it as a "browser", whatever that means.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?rev=78c4d7d788d8#7678

Comment 27

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Is it hitting
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> cpp#7673 ?

I put my logging in the wrong place - it is hitting that :/

I'm not sure what the correct fix would be here to be honest, if we want that work-around in place, then I'm not sure how we can differentiate between a window.open opening something related to the 'app' a window.open to launch a link...

Given that until bug 1060191 these new windows would've gotten entirely new wrappers though, maybe we should disable this IsApp for anything except the original window?

Comment 28

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

Given https://bugzilla.mozilla.org/show_bug.cgi?id=940036#c22 I'm happy to take out this code now (and in 2.1) so that we use the page-specified viewport tag. That should fix this issue.

Comment 29

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

(Note however that there might be other problems caused by these links opening in "app" windows which might still need to be addressed, unless that aspect of the change was intentional)

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Remove b2g app meta-viewport work-around

Excellent, let's get rid of this (confirmed fixes the problem)

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> (Note however that there might be other problems caused by these links
> opening in "app" windows which might still need to be addressed, unless that
> aspect of the change was intentional)

This bug only affects the Facebook app, which is a shallow package of their mobile website (so it is indeed an app). I suppose they shouldn't be using window.open to open external links in an app (though I wonder if it's expected behaviour that a domain change in a new window still counts as being in the app?)

That's a separate issue anyway.

Comment 32

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

(In reply to Chris Lord [:cwiiis] from comment #31)
> I suppose they shouldn't be using
> window.open to open external links in an app (though I wonder if it's
> expected behaviour that a domain change in a new window still counts as
> being in the app?)

I don't know if Fennec webapps are comparable, but there when I have a link that does target="_blank" (which should be equivalent to window.open), it spawns the link in the browser outside the app. If I click on a link to another domain it will load that page in the webapp window directly but have a little statusbar thingy at the top that indicates the domain of the page I'm on. Personally I would expect window.open here to not open the link inside the same app.

Comment 33

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/27624d9e8f27

Comment 34

[Chris Lord [:cwiiis]]

n?alive to comment on comment #32, as it's window-management related. I'm inclined to agree. afaik, that link handling is done Gecko-side though.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cc0208122f

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3223963&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3224196&repo=mozilla-inbound

The above are just what's failed so far. There could be more. Please verify green on Try.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

And mochitest failures:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3221951&repo=mozilla-inbound

Comment 37

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Chris Lord [:cwiiis] from comment #31)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> > (Note however that there might be other problems caused by these links
> > opening in "app" windows which might still need to be addressed, unless that
> > aspect of the change was intentional)
> 
> This bug only affects the Facebook app, which is a shallow package of their
> mobile website (so it is indeed an app). I suppose they shouldn't be using
> window.open to open external links in an app (though I wonder if it's
> expected behaviour that a domain change in a new window still counts as
> being in the app?)
> 
> That's a separate issue anyway.

If I understand the problem correctly,
facebook is always using a "redirect" way to navigate a link;

That is to say, when you click a link to |www.google.com| in facebook app, it will call window.open('http://facebook.com/?redirect=www.google.com').

Before the redirecting happens, the opened window should be considered to be part of the facebook - argee?
But after the page is redirected outside the origin of facebook to |www.google.com|,
we should not treat if it's still inside facebook, but we need to:
* Fix bug 1060191 which is pointed out by Vivien.
* Tell gecko that the page now is not part of the app but more like a webpage.

I wonder if gecko needs gaia to "remove mozApp attribute" to let it know the mozbrowser iframe now becomes a webpage from an app? Do we support that? Or gecko is smart enough to deal with this case...

Comment 38

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

Ah, I didn't realize it was a redirect; that makes more sense then. I don't know if gecko supports removing the mozapp attribute on a window already created, but it does sound like that would be the correct solution here. We should file a follow-up bug to track this outstanding issue.

Comment 39

[Chris Lord [:cwiiis]]

Meanwhile, I assume all the test failures for this are because we have test pages that need viewport tags - I should've anticipated this, sorry to the sheriffs. Will make sure the next is green before pushing.

Comment 40

[Chris Lord [:cwiiis]]

Tbpl link for convenience: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=27624d9e8f27

Comment 41

[Chris Lord [:cwiiis]]

Attachment: Remove b2g app meta-viewport work-around + test fixes

I'm carrying the r+, but unfortunately, this patch still has one failing reftest:

http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-c802f3bdcb8c/try-emulator/try_ubuntu64_vm-b2g-emulator_test-reftest-11-bm117-tests1-linux64-build250.txt.gz&only_show_unexpected=1

Looking at the screen, this seems to hint at some other bug that this exposes, rather than the reftest actually failing (the area of the test matches exactly).

Comment 42

[Chris Lord [:cwiiis]]

At this point, this is getting well outside my area of knowledge and I'm not sure what to investigate next.

n?milan to hand this off to someone on gfx team (though perhaps it's actually a layout problem?)

Comment 43

[Chris Lord [:cwiiis]]

This was the latest try push, for reference: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c802f3bdcb8c

Comment 44

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

So, to be clear, this is the right fix, but we have failing tests, and that's the "only" issue?

Comment 45

[Chris Lord [:cwiiis]]

(In reply to Milan Sreckovic [:milan] from comment #44)
> So, to be clear, this is the right fix, but we have failing tests, and
> that's the "only" issue?

That's correct.

Comment 46

[Chris Lord [:cwiiis]]

I'm not going to assign this to myself just yet, but I'm trying to work around the bug that this seems to expose:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=145b3ff4f860

Comment 47

[Chris Lord [:cwiiis]]

Who am I kidding... Patch incoming.

Comment 48

[Chris Lord [:cwiiis]]

Attachment: Remove b2g app meta-viewport work-around + test fixes v2

Carring r=kats, asking r?roc for the change to layout/reftests/image/background-image-zoom-1-*

I don't believe my change affects what the test is trying to achieve, but it should be reviewed nonetheless.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e880faddd4f7

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8516799 [details] [diff] [review]
Remove b2g app meta-viewport work-around + test fixes v2

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

Looks OK but are we really confident that FxOS apps have <meta viewport>s of their own now?

Comment 50

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> Comment on attachment 8516799 [details] [diff] [review]
> Remove b2g app meta-viewport work-around + test fixes v2
> 
> Review of attachment 8516799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK but are we really confident that FxOS apps have <meta viewport>s of
> their own now?

I've been running with this patch for a week or two and haven't come across any apps that don't - we've given plenty of warning, and our behaviour here is the stand-out vs. every other mobile browser, so I think it's reasonable to make this change now.

Comment 51

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/dffbf0a55955

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Chris Lord [:cwiiis]]

Comment on attachment 8516799 [details] [diff] [review]
Remove b2g app meta-viewport work-around + test fixes v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug
User impact if declined: Very poor usability on most links opened from an app
Testing completed: Tested on dog-food phone for a week or two, passes tests
Risk to taking this patch (and alternatives if risky): Risk of causing some apps to appear zoomed out if they don't adhere to standards (this fixes a long-standing b2g behaviour oddity that apps could have relied on, but that we warned we'd remove a long time ago)
String or UUID changes made by this patch: None

Comment 54

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.2

Actual Results: User can zoom in and out of webpage opened via link from Facebook app.

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141105160209
Gaia: 7918024c737c4570cacd784f267e28737ae05dea
Gecko: 2114ef80f6ae
Gonk: 
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0


Leaving verifyme keyword for uplift to 2.1

Comment 55

[bhavana bajaj [:bajaj]]

(In reply to Brogan Zumwalt [:BroganZ] from comment #54)
> Issue verified fixed on Flame 2.2
> 
> Actual Results: User can zoom in and out of webpage opened via link from
> Facebook app.
> 
> Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
> BuildID: 20141105160209
> Gaia: 7918024c737c4570cacd784f267e28737ae05dea
> Gecko: 2114ef80f6ae
> Gonk: 
> Version: 36.0a1 (2.2 Master)
> Firmware: V188-1
> User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
> 
> 
> Leaving verifyme keyword for uplift to 2.1

I am approving for uplift but Brogan, given he highlighted risk, can we also try testing some other commonly used apps here ?

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/62a455f806ea

Comment 57

[Brogan Zumwalt [Inactive]]

Links to zoomable pages via Marketplace downloaded apps Twitter, SoundCloud, and YouTube show as expected. User can zoom in and out on all pages tested.

Comment 58

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.1

Actual Results: User can zoom in and out of webpage opened via link from Facebook app.

Device: Flame 2.1(319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0