964935 - [B2G] Double tapping on a website that contains a long link always causes the page to zoom out.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Ross Kuhlman ([e-mail])]

The browser is viewing a page that contains RTSP content. If the user double taps anywhere on the page, the page will zoom out. The page can be zoomed out beyond the pinch-to-zoom limit, at which point the user can no longer scroll around the page or utilize pinch-to-zoom.

Repro Steps:
1) Updated Buri to BuildID: 20140127004002
2) Launch Browser app and navigate to http://bit.ly/1bsSLUz.
3) Tap on the search bar and type in a word. (In the logcat, I searched for 'cats')
4) Tap on 'submit search' button.
5) When page populates with video content, double-tap anywhere on the screen.
6) Repeat step 5 several times.

Actual:
Double tapping on the screen causes the page to zoom out. This can be done repeatedly to zoom beyond the limits set by pinch-to zoom, and at this point the page can no longer be scrolled or zoomed.

Expected:
Double tap command toggles between zooming in on the area tapped, and zooming out to the default page width.

Environmental Variables:
Device: Buri v1.3 Moz RIL
BuildID: 20140127004002
Gaia: 25a45a836a4a21a30f63fa7b544b42e8b781180a
Gecko: c40099a42c1f
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Notes:
* Steps 3 and 4 can be skipped, and the issue will still occur.
* Workaround: refreshing the page or navigating to a different page will fix the issue. User can scroll and zoom again.

Repro frequency: 100%
See attached: screenshot, logcat

Comment 1

[Ross Kuhlman ([e-mail])]

Attachment: DoubleTapAlwaysZoomsOut.txt

Comment 2

[Ross Kuhlman ([e-mail])]

Attachment: Pinch_DoubleTap_Comparison.jpg

Comment 3

[Jason Smith [:jsmith]]

Can we find out if this reproduces on 1.1?

Comment 4

[Brogan Zumwalt [Inactive]]

Issue does NOT occur on 1.1

Result: Double-tapping once zooms in on page. Double-tapping once more zooms out again. Repeating these steps produces the same expected result.

Environmental Variables:
Device: Buri v1.1 COM RIL
BuildID: 20140129041220
Gaia: c434fe9a0e823029796805e141cfa983cda2d246
Gecko: 78de4743eec5
Version: 18.0
Firmware Version: V1.2-device.cfg

Comment 5

[Jason Smith [:jsmith]]

What about 1.2?

Comment 6

[Brogan Zumwalt [Inactive]]

Issue does NOT occur on 1.2

Result: Double-tapping once zooms in on page. Double-tapping once more zooms out again. Repeating these steps produces the same expected result.

Environmental Variables:
Device: Buri v1.2 COM RIL
BuildID: 20140131004004
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: f9f469d5d1e1
Version: 26.0
RIL: 01.02.00.019.102
Firmware Version: V1.2-device.cfg

Comment 7

[Jason Smith [:jsmith]]

This sounds like double tapping to zoom in doesn't work in web content correctly here.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> This sounds like double tapping to zoom in doesn't work in web content
> correctly here.

I've confirmed this bug myself. We're doing the inverse of what the user would expect to happen here.

Comment 9

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

Botond, can you take a look if this is in APZC code?

Comment 10

[pfield]

This issue does not seem connected to the APZ developer option. (With the option on or off, it occurs regardless.)

This issue started to occur on the Buri 1.3 Build ID: 20131109040200

Environmental Variables:
Device: BURI 1.3 MOZ
BuildID: 20131109040200
Gaia: 5cf3e2972c4d623ab0759ad85f6c4673ed9ed75f
Gecko: 9e571ad29946
Version: 28.0a1
Firmware Version: V1.2-device.cfg


Last working Buri 1.3 Build ID: 20131108123740

Environmental Variables:
Device: BURI 1.3 MOZ
BuildID: 20131108123740
Gaia: 01800b04cc497db9904ed8e3837187a22ebe47ee
Gecko: f003c386c77a
Version: 28.0a1
Firmware Version: V1.2-device.cfg

Comment 11

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

The double-tap logic is outside of APZ so that makes sense. It is located in BrowserElementPanning.js and is used regardless of whether APZ is enabled or not.

Comment 12

[Jason Smith [:jsmith]]

This is not a browser bug - BrowserElementPanning.js falls under DOM.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Unfortunately it does not appear that we touched BrowserElementPanning.js within that regression range.

Comment 14

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Vincent, not sure if it's related to RTSP.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Wesley Huang [:wesley_huang] from comment #14)
> Vincent, not sure if it's related to RTSP.

This is not related to RTSP - the test case here involves web content using long links & video tags. It could easily be reproduced not using RTSP data at all.

Comment 16

[Preeti Raghunath(:Preeti)]

Andrew,

Please review from DOM

Comment 17

[Punam Dahiya [:pdahiya]]

Here's a link that reproduces the issue of zooming out on double tap to the point where pinch to zoom fails

1. Enter URL https://support.mozillamessaging.com/en-US/kb/thunderbird-and-yahoo

or you can search for below text

"how to thunderbird yahoo" , the links shows up second  in search results

2. Double tap to successfully zoom in

3. Repeat double tap after this, zooms out the page to a point where pinch to zoom fails

Environment 
Device: Hamachi
OS Version: 1.3
Platform- 28.0a2
BuildId: 20140131004001

Comment 18

[Andrew Overholt [:overholt]]

(In reply to pfield from comment #10)
> This issue does not seem connected to the APZ developer option. (With the
> option on or off, it occurs regardless.)
> 
> This issue started to occur on the Buri 1.3 Build ID: 20131109040200
> [...]
> Gecko: 9e571ad29946
> [...]
> Last working Buri 1.3 Build ID: 20131108123740
> Gecko: f003c386c77a

kats opined this *could* be a layout change but there are a number of layout-related commits in this range.

QA, can we get a smaller range, please?

Comment 19

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

Assigning to myself just in case this is a BrowserElementPanning.js / APZC issue.

Comment 20

[Jason Smith [:jsmith]]

A deeper range isn't possible - we don't have pre-built builds in the November timeframe that go deeper than a day.

Comment 21

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> Assigning to myself just in case this is a BrowserElementPanning.js / APZC
> issue.

After investigating it does not seem to be a BrowserElementPanning.js issue as I can reproduce more or less the issue described in comment 0 by pinching. But by pinching we are stucked only sometimes which may be because of the zoom level that is bigger than the one that can be reached by double tapping.

Still investigating.

Comment 22

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

By turning on layer borders there is something really weird.
It ends up that the rectangle covered by APZ is smaller than the size of the page. So if you start to pan inside it then it works, it you starts to pan outside it then it does not work.

Comment 23

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

Attachment: 2014-02-06-17-02-02.png

It may be clearer with a screenshot.

Comment 24

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

Resetting the assignee as this bug is likely due to bug 935219. Asking 1.3? on the root cause.

Comment 25

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

Btw that's really an Async Pan Zoom issue, so let's set the component to the right thing.

Comment 26

[Doug Sherk (:drs) (inactive)]

You don't even have to double tap to get repro this. Just rotating the device to landscape gets it too.

Comment 27

[Doug Sherk (:drs) (inactive)]

When we enter this over-out-scrolled state, the page's CSS scrollable rect is being misreported as ~= the composition bounds in TabChild, even though the page is clearly much longer.

Comment 28

[Doug Sherk (:drs) (inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #24)
> Resetting the assignee as this bug is likely due to bug 935219. Asking 1.3?
> on the root cause.

My investigation has been pretty shallow so far, but I don't see any evidence yet that this has to do with composition bounds being calculated incorrectly (bug 935219). Would you explain why you think this please?

Comment 29

[Doug Sherk (:drs) (inactive)]

OK, I rebased and applied the patch in bug 935219 comment 63, and based on the results, I think that it does indeed fix the problems here. There might be additional problems on top of the ones that patch fixes, but it's very difficult to fix them without bug 935219 landed first. Note that my rebase wasn't perfect so there's definitely room for error in this assessment.

I'm unassigning myself for now, but I'd be happy to retake it if it turns out that there's more to this than just bug 935219.

Comment 30

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

Assigning to Botond because this could be blocked by bug 935219, but we need to understand that first.

Comment 31

[Botond Ballo [:botond]]

This looks like it is indeed caused by bug 935219. Since we include the layer's own resolution in the composition bounds (the thing bug 935219 is fixing), the composition bounds gets smaller as we zoom out. When we constrain, in AsyncPanZoomController::OnScale() the amount by which the user can zoom out to avoid underzoom [1], we calculate the minimum zoom as the ratio of the composition bounds and the scrollable rect. Since the composition bounds keeps getting smaller, so does this minimum, so we effectively allow the user to zoom out indefinitely.

I'll pick up work on bug 935219, it's long overdue anyways. If all goes well, all that remains to be done is to test Timothy's latest patches for the prerequisite bug 959847, and rebase the bug 935219 patch and make sure it works.



[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#854

Comment 32

[Jason Smith [:jsmith]]

bug 935219 isn't a regression & has been around for a long time, so I don't think it has anything to do with the issue here. This issue regressed in the 1.3 timeframe, so we should be studying what regressed in the 1.3 timeframe to cause this issue in the first place.

Comment 33

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f003c386c77a&tochange=9e571ad29946

Comment 34

[Jason Smith [:jsmith]]

Two bugs look suspicious here:

1. bug 902505 - https://hg.mozilla.org/mozilla-central/rev/14366dd910b6
2. bug 935624 - https://hg.mozilla.org/mozilla-central/rev/5040cffe32af & https://hg.mozilla.org/mozilla-central/rev/700ce8c0d14d

Comment 35

[Botond Ballo [:botond]]

(In reply to Jason Smith [:jsmith] from comment #34)
> Two bugs look suspicious here:
> 
> 1. bug 902505 - https://hg.mozilla.org/mozilla-central/rev/14366dd910b6
> 2. bug 935624 - https://hg.mozilla.org/mozilla-central/rev/5040cffe32af &
> https://hg.mozilla.org/mozilla-central/rev/700ce8c0d14d

I'll bet it's the addition of the line that updates mCompositionBounds in NotifyLayersUpdated() in the Part 2 patch of bug 935624 that regressed this. Without that update, the wrong composition bounds that layout calculates is not retained by APZC.

However, removing that update will break other use cases where we get a correct composition bounds and we do want to update it.

I continue to believe that the proper solution is to calculate the composition bounds correctly, i.e. bug 935219. If we land it on trunk, and we observe it for a few days and it doesn't cause regressions, would you reconsider allowing it to be uplifted to 1.3?

Comment 36

[Jason Smith [:jsmith]]

(In reply to Botond Ballo [:botond] from comment #35)
> (In reply to Jason Smith [:jsmith] from comment #34)
> > Two bugs look suspicious here:
> > 
> > 1. bug 902505 - https://hg.mozilla.org/mozilla-central/rev/14366dd910b6
> > 2. bug 935624 - https://hg.mozilla.org/mozilla-central/rev/5040cffe32af &
> > https://hg.mozilla.org/mozilla-central/rev/700ce8c0d14d
> 
> I'll bet it's the addition of the line that updates mCompositionBounds in
> NotifyLayersUpdated() in the Part 2 patch of bug 935624 that regressed this.
> Without that update, the wrong composition bounds that layout calculates is
> not retained by APZC.
> 
> However, removing that update will break other use cases where we get a
> correct composition bounds and we do want to update it.
> 
> I continue to believe that the proper solution is to calculate the
> composition bounds correctly, i.e. bug 935219. If we land it on trunk, and
> we observe it for a few days and it doesn't cause regressions, would you
> reconsider allowing it to be uplifted to 1.3?

Let's land this first on trunk & do some exploratory testing to see what falls out from it. If you guys give input on what you think could regress from that patch, then that would help. After we complete around testing this, then we can re-evaluate what to do here for 1.3.

Comment 37

[Chris Lee [:clee]]

> Let's land this first on trunk & do some exploratory testing to see what
> falls out from it. If you guys give input on what you think could regress
> from that patch, then that would help. After we complete around testing
> this, then we can re-evaluate what to do here for 1.3.

Sounds like the right approach. Please update when we have additional data once patch lands on master.  Thanks.

Comment 38

[Jason Smith [:jsmith]]

(In reply to Botond Ballo [:botond] from comment #35)
> (In reply to Jason Smith [:jsmith] from comment #34)
> > Two bugs look suspicious here:
> > 
> > 1. bug 902505 - https://hg.mozilla.org/mozilla-central/rev/14366dd910b6
> > 2. bug 935624 - https://hg.mozilla.org/mozilla-central/rev/5040cffe32af &
> > https://hg.mozilla.org/mozilla-central/rev/700ce8c0d14d
> 
> I'll bet it's the addition of the line that updates mCompositionBounds in
> NotifyLayersUpdated() in the Part 2 patch of bug 935624 that regressed this.
> Without that update, the wrong composition bounds that layout calculates is
> not retained by APZC.
> 
> However, removing that update will break other use cases where we get a
> correct composition bounds and we do want to update it.

The lingering questions I think I want to understand more is:

Why is intermittently being correct in the calculation here better than not including the calculation at all? If we know we're not always going to be right, then aren't we making things worse here? 1.2 didn't include this calculation, so why does 1.3 need it, even it means we're not always going to be correct?

I guess I'm still having trouble seeing why a backout is a bad idea.

Comment 39

[Botond Ballo [:botond]]

(In reply to Jason Smith [:jsmith] from comment #38)
> (In reply to Botond Ballo [:botond] from comment #35)
> > I'll bet it's the addition of the line that updates mCompositionBounds in
> > NotifyLayersUpdated() in the Part 2 patch of bug 935624 that regressed this.
> > Without that update, the wrong composition bounds that layout calculates is
> > not retained by APZC.
> > 
> > However, removing that update will break other use cases where we get a
> > correct composition bounds and we do want to update it.
> 
> The lingering questions I think I want to understand more is:
> 
> Why is intermittently being correct in the calculation here better than not
> including the calculation at all? If we know we're not always going to be
> right, then aren't we making things worse here? 1.2 didn't include this
> calculation, so why does 1.3 need it, even it means we're not always going
> to be correct?
> 
> I guess I'm still having trouble seeing why a backout is a bad idea.

It's not so much that the composition bounds layout calculates is _wrong_, but rather that it's in the wrong coordinate system. 

I drew a diagram illustrating the various coordinate systems that are in play and their relationships [1]. You can see on the diagram that the coordinate system APZC _thinks_ the composition bounds are in (which I've called "L's screen pixels", where L is the layer being scrolled) is different from the coordinate system that the composition bounds are _really_ in (which I've called "sillier pixels").

These two coordinate systems sometimes coincide, and sometimes they are different:

  - root frame, no zoom    - coincide
  - root frame, zoomed in  - coincide
  - root frame, zoomed out - different
  - subframe, no zoom      - coincide
  - subframe, zoomed in    - N/A (we don't support zooming subframes)
  - subframe, zoomed out   - N/A (we don't support zooming subframes)

In the scenario where we have a root frame that is zoomed out, the coordinate systems are different and we can get incorrect behaviour, like in this bug.

The code that updates mCompositionBounds in NotifyLayersUpdated() accounts for changes in the composition bounds that can happen for various reasons:

  (1) the frame being zoomed in
  (2) the frame being zoomed out
  (3) an ancestor frame being zoomed (in or out)
  (4) the screen orientation changes
  (5) DOM changes the size of the scrollable element

We can ignore case (1) because the root frame is the only one that can be zoomed, and the root frame's composition bounds are clamped to the widget bounds so they don't change when the root frame is zoomed in.

In case (2), APZC gets the "wrong" value, giving rise to this bug.

In cases (3), (4), and (5), APZC is correctly notified of a change it needs to know about.

I believe removing the update of mCompositionBounds in NotifyLayersUpdated() would fix case (2) (i.e. this bug), but break cases (3), (4), and (5). It's not my call to say whether that would be better or worse.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8380975

Comment 40

[Jason Smith [:jsmith]]

Bhavana & I talked about this in person - we think the safest option is to backout the regressing patch here on 1.3, as that puts us back to a state that users are already used to for 1.2. While this loses out on some of the enhancements that the patch introduced in some cases, we believe that the fallout the patch introduces places the user into potential situations that they worked in a past release, but no longer work in 1.3. As a result, we think this risks introducing user frustration with 1.3, as they'll get frustrated about something they saw working previously no longer working. The backout also looks not too risky based on complexity of the patch seen in https://bugzilla.mozilla.org/attachment.cgi?id=828141&action=diff.

Chris & Milan - Do you agree or disagree with the suggested path forward?

Comment 41

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

For the record, I am strongly opposed to this. That patch landed over three months ago in some of the most heavily-modified code that we have. Tons of stuff we have landed since then depends on that code working the way it is now. Backing that out, even if it's clean from a "diff" point of view, is going to break all sorts of things.

Comment 42

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

(In reply to Jason Smith [:jsmith] from comment #40)
> we think the safest option is to
> backout the regressing patch here on 1.3, as that puts us back to a state
> that users are already used to for 1.2.

Note that this sentence would be true if nothing landed since that patch that depends on it. But that is not the case.

Comment 43

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #42)
> (In reply to Jason Smith [:jsmith] from comment #40)
> > we think the safest option is to
> > backout the regressing patch here on 1.3, as that puts us back to a state
> > that users are already used to for 1.2.
> 
> Note that this sentence would be true if nothing landed since that patch
> that depends on it. But that is not the case.

So what are the hard dependencies on that patch then (e.g. what bugs specifically)?

Comment 44

[Botond Ballo [:botond]]

(In reply to Jason Smith [:jsmith] from comment #43)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #42)
> > (In reply to Jason Smith [:jsmith] from comment #40)
> > > we think the safest option is to
> > > backout the regressing patch here on 1.3, as that puts us back to a state
> > > that users are already used to for 1.2.
> > 
> > Note that this sentence would be true if nothing landed since that patch
> > that depends on it. But that is not the case.
> 
> So what are the hard dependencies on that patch then (e.g. what bugs
> specifically)?

A significant one is bug 950487, which removed AsyncPanZoomController::UpdateCompositionBounds(). This was the mechanism used in 1.2 to update the composition bounds stored by APZC in cases like (4) and (5) from comment #39. Its removal relied on the fact that bug 935624 added the update of the composition bounds in NotifyLayersUpdated().

There are probably others, but I can't think of them off the top of my head.

Comment 45

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

That is the big one, yes. If we were to back out bug 935624 nothing would update the composition bounds anymore, and all sorts of things would break. For instance, you'd probably end up allowing a screenful of overscroll because the Axis code that figures out how far you're allowed to scroll would assume a zero size for the composition bounds.

Also, I'm not about to go through 3 months of changes to APZC code and try to figure out exactly what depended on that change - that would take a week or more and have at best 50% accuracy. It would be faster to just back it out locally and see for yourself how much stuff is broken.

Comment 46

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

I'm going to take this off Botond's hands for now so he can focus on getting other things done.

Comment 47

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

Also, putting back the dependency on bug 935219. The dependency is a technical one. I don't care if bug 935219 has tracking/blocking flags set or not but it should fix this issue and so it blocks this bug from a code point of view.

Comment 48

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #47)
> Also, putting back the dependency on bug 935219. The dependency is a
> technical one. I don't care if bug 935219 has tracking/blocking flags set or
> not but it should fix this issue and so it blocks this bug from a code point
> of view.

Kats, it was my understanding that https://bugzilla.mozilla.org/attachment.cgi?id=828141&action=diff was something easier to back out, but looks like that's not the case based on last few comments. 

Options we have here to fix the issue, uplift 935219. But the concern I have here are the uplift of its dependencies (https://bugzilla.mozilla.org/show_bug.cgi?id=959847) are a lot of change's..Can you confirm that all the patches in the dependency are really needed ? Or, are you planning on rebasing 935219 for v1.3 to mitigate risk here ? Irrespective, we will need QA help to do targeted testing here to identify fallouts but would like to go the lowest risk way for 1.3 if we have a choice.

Comment 49

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

(In reply to bhavana bajaj [:bajaj] from comment #48)
> Kats, it was my understanding that
> https://bugzilla.mozilla.org/attachment.cgi?id=828141&action=diff was
> something easier to back out, but looks like that's not the case based on
> last few comments. 

Yeah, I don't think it's easy to back that out.

> Options we have here to fix the issue, uplift 935219. But the concern I have
> here are the uplift of its dependencies
> (https://bugzilla.mozilla.org/show_bug.cgi?id=959847) are a lot of
> change's..Can you confirm that all the patches in the dependency are really
> needed ?

We might be able to skip some of those patches as long as we are only uplifting to b2g28 and therefore don't care about breaking Metro/Fennec. Botond and Timothy would have to weigh in on that.

Comment 50

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> (In reply to bhavana bajaj [:bajaj] from comment #48)
> > Kats, it was my understanding that
> > https://bugzilla.mozilla.org/attachment.cgi?id=828141&action=diff was
> > something easier to back out, but looks like that's not the case based on
> > last few comments. 
> 
> Yeah, I don't think it's easy to back that out.
> 
> > Options we have here to fix the issue, uplift 935219. But the concern I have
> > here are the uplift of its dependencies
> > (https://bugzilla.mozilla.org/show_bug.cgi?id=959847) are a lot of
> > change's..Can you confirm that all the patches in the dependency are really
> > needed ?
> 
> We might be able to skip some of those patches as long as we are only
> uplifting to b2g28 and therefore don't care about breaking Metro/Fennec.
> Botond and Timothy would have to weigh in on that.

I think we may not need any of the bug 959847 patches for B2G, they are only needed for Metro. Timothy can confirm this with more confidence.

Comment 51

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #50)
> I think we may not need any of the bug 959847 patches for B2G, they are only
> needed for Metro. Timothy can confirm this with more confidence.

I don't _think_ we need those patches for b2g. I don't know enough about the structure of b2g to say with certainty. The question we need to answer is "Is there any place in b2g where we want to be able to pan _and_ zoom a document that is not a display root document (root document in it's process)?"

Comment 52

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #51)
> The question we need to answer is
> "Is there any place in b2g where we want to be able to pan _and_ zoom a
> document that is not a display root document (root document in it's
> process)?"

I believe the answer is no. APZ doesn't currently support zooming subdocuments, and I believe on B2G all root documents are display root documents because all 'mozasyncpanzoom' iframes are also 'remote'.

Comment 53

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #52)
> (In reply to Timothy Nikkel (:tn) from comment #51)
> > The question we need to answer is
> > "Is there any place in b2g where we want to be able to pan _and_ zoom a
> > document that is not a display root document (root document in it's
> > process)?"
> 
> I believe the answer is no. APZ doesn't currently support zooming
> subdocuments, and I believe on B2G all root documents are display root
> documents because all 'mozasyncpanzoom' iframes are also 'remote'.

I mean, "all _APZ-enabled_ root documents are display root documents because..."

Comment 54

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

If we come up with something that can be uplifted, make sure Fabrice OK's it and we get a specific approval.  Cherry picking out changes always has potential for nasty problems.

Comment 55

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #52)
> APZ doesn't currently support zooming subdocuments

Sorry, I didn't phrase that accurately. APZ doesn't support zooming scrollable subframes.

So if you have a root document A, and a subdocument B:
  (1) if A is scrollable (i.e. has an APZC), we do not support zooming B
  (2) if A does not have an APZC, there is no problem with zooming B

I believe my conclusion about never wanting to zoom a document that is not a display root document on B2G holds, because the display root document always gets an APZC, so any subdocument within it would fall into case (1) above.

Comment 56

[[:fabrice] Fabrice Desré]

I think we need to try it this way:
- kats provides the cherry pick we need for 1.3
- we spun a build for QA with this changes
- if QA smoketest is ok, we'll land.

Comment 57

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

Fabrice, having Kats, or for that matter Timothy or Botond, spend time on this would jeopardize the 1.4 targets we have as blockers for bug 960372. That's a decision that would need to be authorized above me.

Comment 58

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #57)
> Fabrice, having Kats, or for that matter Timothy or Botond, spend time on
> this would jeopardize the 1.4 targets we have as blockers for bug 960372.
> That's a decision that would need to be authorized above me.

1.4 is lower priority than 1.3 work, especially given the fact that IOT is active in 1.3. We've already been a decision here - we need a contextual 1.3 patch here to resolve this bug & it is to remain as a blocker. Please assign.

Comment 59

[Doug Sherk (:drs) (inactive)]

(In reply to Milan Sreckovic [:milan] from comment #57)
> Fabrice, having Kats, or for that matter Timothy or Botond, spend time on
> this would jeopardize the 1.4 targets we have as blockers for bug 960372.
> That's a decision that would need to be authorized above me.

I'm not the best person to work on this and I have my own 1.4 blockers, but I may be able to take it if there's nobody else available. I think with a briefing from Botond I'll be ok. I don't know the drop-dead date for this though, and I probably can't spare any time until next week.

Comment 60

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

Sounds like Doug can get to it next week and will talk to Botond about it.

Comment 61

[Doug Sherk (:drs) (inactive)]

After talking with Botond some more, I don't think it makes sense for me to take this. He thinks that the fix for it is to just uplift bug 935219. He also can't think of a quick fix for 1.3, and for me to be at the same level as him would take me a while of familiarizing myself. So it's unlikely that I'll come up with a less risky way of fixing this, and probably waste a bunch of time in the process.

Comment 62

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

Thanks.  1.3? to get attention in the triage meeting.  We don't appear have a practical solution for this problem in the 1.3 timeframe.  That may change once we have a fix for bug 935219.

Comment 63

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #62)
> Thanks.  1.3? to get attention in the triage meeting.  We don't appear have
> a practical solution for this problem in the 1.3 timeframe.  That may change
> once we have a fix for bug 935219.

We are not punting this bug. Please provide a fix this in the 1.3 timeframe - we've already made a determination that we can't ship with this bug.

Comment 64

[Jason Smith [:jsmith]]

(In reply to Doug Sherk (:drs) from comment #61)
> After talking with Botond some more, I don't think it makes sense for me to
> take this. He thinks that the fix for it is to just uplift bug 935219. He
> also can't think of a quick fix for 1.3, and for me to be at the same level
> as him would take me a while of familiarizing myself. So it's unlikely that
> I'll come up with a less risky way of fixing this, and probably waste a
> bunch of time in the process.

We are not taking a full uplift of bug 935219. There was already discussion made here that a less risky fix was possible, so please find an assignee to move forward to provide a contextual 1.3 fix here that mitigates the risk as much as possible here.

Remember that we are held to a high bar for quality with the browser experience on Firefox OS. Putting ourselves in situations where APZC functionality stops working in portion of web content is not acceptable. We've already discussed this extensively that this must be fixed to preserve an effective browsing experience in the 1.3 timeframe.

Comment 65

[Botond Ballo [:botond]]

I talked to Jason on IRC. Since we established in comment 51 and comment 52 that bug 959847 isn't necessary for B2G, the only thing that would need to be uplifted to 1.3 is the bug 935219 patch itself, which Jason indicated would be OK.

Comment 66

[bhavana bajaj [:bajaj]]

(In reply to Botond Ballo [:botond] from comment #65)
> I talked to Jason on IRC. Since we established in comment 51 and comment 52
> that bug 959847 isn't necessary for B2G, the only thing that would need to
> be uplifted to 1.3 is the bug 935219 patch itself, which Jason indicated
> would be OK.

Thanks, please make sure to have a build spun for QA to do smoketest/exploratory testing here before we land this patch on 1.3.

Comment 67

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

Assign to me as a placeholder until the plates of other engineers clear up.

Comment 68

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

Bug 935219 has an 83kb patch. Botond, you asked for it :) let's get the uplift patch.

Comment 69

[Sandip Kamat]

PM triage: Need to fix it for 1.3. Stays 1.3+

Comment 70

[Botond Ballo [:botond]]

(In reply to bhavana bajaj [:bajaj] from comment #66)
> (In reply to Botond Ballo [:botond] from comment #65)
> > I talked to Jason on IRC. Since we established in comment 51 and comment 52
> > that bug 959847 isn't necessary for B2G, the only thing that would need to
> > be uplifted to 1.3 is the bug 935219 patch itself, which Jason indicated
> > would be OK.
> 
> Thanks, please make sure to have a build spun for QA to do
> smoketest/exploratory testing here before we land this patch on 1.3.

Bug 935219 has now landed on master. I also posted a 1.3 uplift patch there.

Here is a build for QA to do the smoketest/exploratory testing: http://people.mozilla.org/~bballo/b2g-28.0.en-US.android-arm.tar.gz. Jason, could you assign someone to do this testing?

Comment 71

[Jason Smith [:jsmith]]

Sure. Switching needinfo to Sarah here to find someone to do exploratory testing with the target build.

Comment 72

[Matthew Vaughan]

Attachment: Video of results from exploratory testing.

I tested this using the STR from comment 0 using the target b2g from comment 70 on the 03/12/14 1.3 build. I wasn't able to use zoom at all using the URL from comment 17, even when using the mentioned 01/31/14 1.3 build, so I did not use the webpage. Just for a little more coverage, I used http://www.reddit.com AND had the reporter try out double-tapping.

On http://bit.ly/1bsSLUz I found that double-tapping when the results first load causes them to zoom out a little, and zoom in (back to the original zoom when first loading) after double-tapping a second time.

If you manually zoom in a fair amount and then double-tap, the page will zoom out back to its original zoom when first loading. 

I have also found that the page will be zoomed in on and scrolled down to the next item in the results list if tapping on the center of the screen. This looks like it is due to my taps being close to the next item and is an expected result of double-tapping in this way.

Finally, rotating the phone to landscape orientation produces essentially the same results. The only difference is that double-tapping the first time will zoom the page in a little, double-tapping a second time will zoom in a little more, and double-tapping a third time will zoom the page out a little but not to what it was when first rotating the phone. Rotating back to portrait causes no noticeable issues.

Overall, the reporter and I feel this to be a big improvement.

I attached a video showing the outcome of most of my testing (just doesn't show landscape).

Comment 73

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

That looks fixed to me. I'd still like to wait a few days for the composition bounds changes to bake on central before uplifting.

Comment 74

[Botond Ballo [:botond]]

Attachment: Bug 935219 patch rebased to 1.3

The patch for bug 935219 has now been on trunk for most of this week, and we haven't heard about any B2G regressions from it. Requesting approval to uplift it to fix this bug.

NOTE: This flag is now for security issues only. 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 #): APZ
User impact if declined: Double-tap-to-zoom behaviour is incorrect. Also potentially other things that bug 935219 fixes, like panning when zoomed out.
Testing completed: Locally by me, and by QA (comment 72).
Risk to taking this patch (and alternatives if risky): Moderate. The patch touches key parts of APZ code like hit testing and zooming. However, it has been baking for a long time, and it has been on trunk for almost a week with no reported regressions.
String or UUID changes made by this patch: None.

Comment 75

[Andreas Gal :gal]

Comment on attachment 8391486 [details] [diff] [review]
Bug 935219 patch rebased to 1.3

Please land on 1.3 and 1.3T.

Comment 76

[Botond Ballo [:botond]]

Requesting checkin as per comment 75.

Comment 77

[[:fabrice] Fabrice Desré]

That will end up on the right trees thanks to the magic of our sheriffs.

Comment 78

[Ryan VanderMeulen [:RyanVM]]

Actually, I only found this by accident by way of another bug. We typically only look for FIXED bugs in need of uplift. My understanding is that this was fixed on trunk (v1.4) by other bugs, so marking its status as such. Please reopen the bug if that's not the case.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a97ddd59ff92

Comment 79

[Ryan VanderMeulen [:RyanVM]]

Realized this landed with the wrong bug number. Fixed:
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e2c448e8e3b4