Closed Bug 964935 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Panning and Zooming, defect)

28 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: rkuhlman, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: dogfood1.3,)

Attachments

(5 files)

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
Component: Video/Audio → General
Keywords: qawanted
Product: Core → Firefox OS
Can we find out if this reproduces on 1.1?
QA Contact: bzumwalt
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
QA Contact: bzumwalt
Keywords: qawanted
What about 1.2?
Keywords: qawanted
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
Keywords: qawanted
This sounds like double tapping to zoom in doesn't work in web content correctly here.
blocking-b2g: --- → 1.3?
Component: General → Panning and Zooming
Product: Firefox OS → Core
Version: unspecified → 28 Branch
(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.
Botond, can you take a look if this is in APZC code?
Assignee: nobody → botond
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(botond)
QA Contact: 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
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.
Assignee: botond → nobody
Component: Panning and Zooming → Gaia::Browser
Flags: needinfo?(botond)
Product: Core → Firefox OS
Version: 28 Branch → unspecified
This is not a browser bug - BrowserElementPanning.js falls under DOM.
Component: Gaia::Browser → DOM
Product: Firefox OS → Core
Version: unspecified → 28 Branch
Unfortunately it does not appear that we touched BrowserElementPanning.js within that regression range.
Vincent, not sure if it's related to RTSP.
Flags: needinfo?(vchang)
(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.
Andrew,

Please review from DOM
Flags: needinfo?(overholt)
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
(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?
Flags: needinfo?(overholt)
Keywords: qawanted
Assigning to myself just in case this is a BrowserElementPanning.js / APZC issue.
Assignee: nobody → 21
QA Contact: pfield → pbylenga
A deeper range isn't possible - we don't have pre-built builds in the November timeframe that go deeper than a day.
Keywords: qawanted
(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.
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.
Attached image 2014-02-06-17-02-02.png
It may be clearer with a screenshot.
Flags: needinfo?(botond)
Resetting the assignee as this bug is likely due to bug 935219. Asking 1.3? on the root cause.
Assignee: 21 → nobody
Depends on: 935219
Btw that's really an Async Pan Zoom issue, so let's set the component to the right thing.
Component: DOM → Panning and Zooming
Flags: needinfo?(vchang)
Assignee: nobody → bugzilla+drs
You don't even have to double tap to get repro this. Just rotating the device to landscape gets it too.
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.
(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?
Flags: needinfo?(21)
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.
Assignee: drs+bugzilla → nobody
Flags: needinfo?(21)
Assigning to Botond because this could be blocked by bug 935219, but we need to understand that first.
Assignee: nobody → 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
Flags: needinfo?(botond)
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.
No longer depends on: 935219
(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?
(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.
> 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.
(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.
Flags: needinfo?(botond)
See Also: → 975962
(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
Flags: needinfo?(botond)
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?
Flags: needinfo?(milan)
Flags: needinfo?(clee)
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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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)?
Flags: needinfo?(milan)
Flags: needinfo?(clee)
(In reply to Jason Smith [:jsmith] from comment #43)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
I'm going to take this off Botond's hands for now so he can focus on getting other things done.
Assignee: botond → bugmail.mozilla
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.
Depends on: 935219
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(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)?"
Flags: needinfo?(tnikkel)
(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'.
(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..."
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.
(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.
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.
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.
Assignee: bugmail.mozilla → nobody
(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.
Flags: needinfo?(milan)
(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.
Sounds like Doug can get to it next week and will talk to Botond about it.
Assignee: nobody → drs+bugzilla
Flags: needinfo?(milan)
Whiteboard: dogfood1.3 → dogfood1.3, [ETA: 3/7]
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.
Assignee: drs+bugzilla → nobody
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.
blocking-b2g: 1.3+ → 1.3?
Whiteboard: dogfood1.3, [ETA: 3/7] → dogfood1.3, [ETA: n/a]
(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.
blocking-b2g: 1.3? → 1.3+
Summary: [B2G][RTSP]Double tapping on a website that contains a RTSP video always causes the page to zoom out. → [B2G][RTSP]Double tapping on a website that contains a long link always causes the page to zoom out.
(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.
Summary: [B2G][RTSP]Double tapping on a website that contains a long link always causes the page to zoom out. → [B2G] Double tapping on a website that contains a long link always causes the page to zoom out.
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.
(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.
Assign to me as a placeholder until the plates of other engineers clear up.
Assignee: nobody → milan
Bug 935219 has an 83kb patch. Botond, you asked for it :) let's get the uplift patch.
Assignee: milan → botond
PM triage: Need to fix it for 1.3. Stays 1.3+
(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?
Flags: needinfo?(jsmith)
Sure. Switching needinfo to Sarah here to find someone to do exploratory testing with the target build.
Flags: needinfo?(jsmith) → needinfo?(sparsons)
Keywords: qawanted
Flags: needinfo?(sparsons)
QA Contact: pbylenga → mvaughan
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).
Keywords: qawanted
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.
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.
Attachment #8391486 - Flags: approval-mozilla-b2g28?
Comment on attachment 8391486 [details] [diff] [review]
Bug 935219 patch rebased to 1.3

Please land on 1.3 and 1.3T.
Attachment #8391486 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Requesting checkin as per comment 75.
Keywords: checkin-needed
That will end up on the right trees thanks to the magic of our sheriffs.
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: dogfood1.3, [ETA: n/a] → dogfood1.3,
Target Milestone: --- → 1.4 S4 (28mar)
Realized this landed with the wrong bug number. Fixed:
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e2c448e8e3b4
You need to log in before you can comment on or make changes to this bug.