975962 - [Sora][Browser]I can not zoom in or zoom out in email account ,such as 163 ,hotmail

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[sync-1]

Mozilla build ID: 20140208004002
 
 Created an attachment (id=643902)
 qxdm LOG
 
 DEFECT DESCRIPTION:
 when you lanuch the email account ,you will find the interface can not zoom,the characters is very small ,phenomenon of Beetle lite FF and soul3.5 FF is not the same
 REPRODUCING PROCEDURES:
 1.lanuch "www.baidu.com" ,serach "hotmail denglu" ,to lanuch hotmail account 
 2.when you lancun sucessfully,then zoom in or zoom out  
 3.you can not zoom in or zoom out --KO
 EXPECTED BEHAVIOUR: 
 you can zoom in or out the interface 
 ASSOCIATE SPECIFICATION:
 TEST PLAN REFERENCE:
 TOOLS AND PLATFORMS USED:
 USER IMPACT:
 
 REPRODUCING RATE:
 100%
 
 For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: qxdm LOG

Comment 2

[Jason Smith [:jsmith]]

Can you include a video? I think this might be related, if not a dupe of, bug 964935. A video will confirm that.

Comment 3

[buri.blff]

Attachment: index.png

Can't zoom in and zoom out the page by sliding two fringers.

Comment 4

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

What device is this reproducible on, other than Sora?  The graphics team doesn't have access to Sora.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> What device is this reproducible on, other than Sora?  The graphics team
> doesn't have access to Sora.

A bug like this will reproduce across all devices. I doubt it's device-specific.

Comment 6

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

In that case please take [Sora] out of the summary. I've been explicitly ignoring anything with [Sora] in the summary on the assumption that I shouldn't care about it.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> In that case please take [Sora] out of the summary. I've been explicitly
> ignoring anything with [Sora] in the summary on the assumption that I
> shouldn't care about it.

We aren't going to do that. That title is added to indicate that this was filed originally from the Sora device to help the relevant TAM & partners find their bugs.

Comment 8

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

Martijn, would you mind reproducing this on Hamachi, verify the STR and see if you can get a regression range?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Martijn, would you mind reproducing this on Hamachi, verify the STR and see
> if you can get a regression range?

Talked to Martijn in IRC about this - I'll have someone from Sarah's team look into this.

Comment 10

[Sarah Parsons]

Ok this is is being addressed by jzimbrick.

Comment 11

[J Zimbrick]

Regression Window:

Last Working Environmental Variables:
Device: Buri
BuildID: 20140122004001
Gaia: 744fb691c2b2a25a07c5d19fabf5748ae9aba4d9
Gecko: 71a8786c3815
Version: 28.0a2
Base Image: V1.2-device.cfg

First Broken Environmental Variables:
Device: Buri 
BuildID: 20140123004001
Gaia: 6fbeac2415f07f10de181f0877ddf67ee299b885
Gecko: e8f6bdf8db3d
Version: 28.0a2
Base Image: V1.2-device.cfg

Last Working Gaia/First Broken Gecko: Issue DOES Reproduce
Gaia: 744fb691c2b2a25a07c5d19fabf5748ae9aba4d9
Gecko: e8f6bdf8db3d

First Broken Gaia/Last Working Gecko: Issue Does NOT Reproduce
Gaia: 6fbeac2415f07f10de181f0877ddf67ee299b885
Gecko: 71a8786c3815

Note: At least for Outlook emails the searching mentioned in step 1 of Comment 0 is not necessary to reproduce this issue. This can be reproduced by going directly to hotmail.com, rather than searching for hotmail on baidu. Have not checked 163.

Comment 12

[Jason Smith [:jsmith]]

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=71a8786c3815&tochange=e8f6bdf8db3d

Comment 13

[Jason Smith [:jsmith]]

Might have been caused by bug 950488. If not that bug, then maybe bug 958549.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #13)
> Might have been caused by bug 950488. If not that bug, then maybe bug 958549.

Definitely caused by bug 950488 actually. I'm seeing mention of preventing zooming on overflow:hidden frames in bug 950488. The source of the attached image does include overflow:hidden usage.

Comment 15

[Jason Smith [:jsmith]]

FWIW - This works fine on FxAndroid with a FxOS user agent spoof.

Comment 16

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

Kats, is this page wrong, or is our logic from bug 950488 flawed/incomplete? (or is there a third option)

Comment 17

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

I don't think the logic in bug 950488 is wrong, and I think this is the expected result from that change. However it may be that the behaviour we thought was correct is not. Fennec currently does allow scrolling of overflow:hidden content, but there's a bug on file (886969) to remove that behaviour. That was based on the argument at [1]. There's no real spec for this behaviour though, and other browsers I tested on Android all seem to allow scrolling of overflow:hidden content. Desktop browsers do not.

I'm not sure what the right thing to do here is but I do know that our Gaia apps make heavy use of overflow:hidden and if we allow scrolling of that content it's going to result in a lot of other bugs.

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

Comment 18

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> I'm not sure what the right thing to do here is but I do know that our Gaia
> apps make heavy use of overflow:hidden and if we allow scrolling of that
> content it's going to result in a lot of other bugs.

To be clear, backing out bug 950488 will not by itself break the Gaia apps, because we had a different implementation to prevent scrolling before that landed, and the backout would just restore that old implementation.

So I guess we could back that out and find a different way to fix 950488.

Comment 19

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

Attachment: "Backout" of 950488

It might also be worth retesting bug 950488 with a backout patch to see if it still happens. It might already have been fixed by something else. I'm attaching a patch that should have the same effect as a full backout of bug 950488 but is much lower risk (the rest of the patch was mostly plumbing that can remain in place but just won't be used any more).

Comment 20

[Jason Smith [:jsmith]]

Need input before making a blocking decision:

1. Need to understand the fallout of backing out bug 950488 as a stand alone
2. How common is this use case on the web?

Comment 21

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

2. There are some suggestions in bug 950488, but I don't think anybody investigated this.

1. Fallout from not shipping with bug 950488 un-fixed (as in, removing the blocker status from it), or fallout from doing the work to back out 950488?

The first interpretation of the question - somebody else can better answer this, but I know that a lot of things were hinging on overflow:hidden.  Further, bug 950488 is tagged as 1.3 CS blocker.

Work to do the back out and deal with the consequences?  The original solution took a week to develop.  Looking at one of the files modified, since then, about 25 bugs have been fixed by modifying that same file.  Those 25 bugs would have to be re-verified, and while I wouldn't venture into estimating how many would regress because of the backout, I'd guess that number would be non-zero.  All told, realistically it would likely be more than a week from start to finish.

Comment 22

[Jason Smith [:jsmith]]

My main concern with this bug isn't the problem with the bug itself, but rather the web compatibility fallouts. If the main typical use case is to allow zooming in overflow:hidden frames in the large majority of the web and we aren't doing that, then we'll end up with sites being designed incorrectly to use overflow:hidden under the assumption that zooming will work. As such, FxOS will end up with a broken site experience in these cases. Note that we may be doing the right workflow here, but the typical use case doesn't do that. We also don't have commanding marketshare to influence mobile site design, so we're stuck usually having to follow decisions that focus primarily on getting the mobile web to work, rather than specifying how we want the mobile web to work.

See https://bugzilla.mozilla.org/show_bug.cgi?id=921014#c11 for more information also where Andreas clarified this requirement.

Comment 23

[Jason Smith [:jsmith]]

Karl - Do you have any idea from your web compatibility studies on how often overflow:hidden frames are used across top sites in the mobile web?

Comment 24

[Karl Dubost💡 :karlcow]

jason,

No specific data. It's difficult to test given that the property can be defined on any selectors which could match a frame or not. But let's say first if it's happening and hoping that people call the selector something related to frames (many assumptions)

https://github.com/search?q=overflow%3Ahidden+frame&type=Code&ref=searchresults

we can guess it's happening, I'm not sure if it's frequent and/or create usability issues.

The spec says:

> hidden
>    This value indicates that the content is clipped 
>    and that no scrolling mechanism should be provided 
>    to view the content outside the clipping region. 

And scrolling mechanism is defined as 
http://dev.w3.org/csswg/css-box/#scrolling-mechanism

> The scrolling mechanism depends on the UA. The most 
> common mechanism is a scrollbar, but panners, hand 
> cursors, page flickers, etc. are also possible.

For the zooming part it is an interesting issue. Maybe David Baron has an idea about it. I wonder if it has been discussed on CSS Working Group mailing list.

summary:
Should the content of an iframe with a overflow:hidden be "zoom-able".

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I would expect the overflow:hidden elements not to be zoomable along with their parent -- i.e., panning or zooming gestures inside them would cross the overflow:hidden boundary, and pan/zoom what is inside and outside of the overflow:hidden element identically.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Oh, and I realize I wasn't thinking specifically about frames/iframes when I wrote the previous comment, but I think it still holds for them.  Except, of course, when viewports are involved.)

Comment 27

[Jason Smith [:jsmith]]

Okay - sounds like we're doing the right behavior here, although I don't necessarily like that we're in a situation with the majority of the browsers doing the opposite behavior. We need do some TE work here to ensure this overflow:hidden behavior is well understood. And we really need to get Fennec support soon, as this is kinda bad that one mobile gecko browser supports this, while the other does not.

I'm pulling the nom & bumping this over to TE mobile to do the evangelism work here.

Comment 28

[Karl Dubost💡 :karlcow]

Jason,

I think there is a need of a new bug which is the counterpart of:
Bug 886969 - Fennec allows scrolling of pages with overflow:hidden on the body element

Something like:
"Firefox Android doesn't allow zoom on elements with overflow:hidden."
With a reduced test case and a call to compare on what is happening on other mobile browsers (Chrome, Safari, IEMobile, etc.)

Comment 29

[Botond Ballo [:botond]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #26)
> (Oh, and I realize I wasn't thinking specifically about frames/iframes when
> I wrote the previous comment, but I think it still holds for them.  Except,
> of course, when viewports are involved.)

(In reply to Jason Smith [:jsmith] from comment #27)
> Okay - sounds like we're doing the right behavior here, although I don't
> necessarily like that we're in a situation with the majority of the browsers
> doing the opposite behavior. We need do some TE work here to ensure this
> overflow:hidden behavior is well understood. And we really need to get
> Fennec support soon, as this is kinda bad that one mobile gecko browser
> supports this, while the other does not.
> 
> I'm pulling the nom & bumping this over to TE mobile to do the evangelism
> work here.

I believe we are in the "Except when viewports are involved" case (if the
page used to be zoomable and now isn't, the overflow:hidden must be on
the html element, i.e., the viewport), and therefore we're _not_ doing the
right behaviour here (and the majority of the browsers are).

Comment 30

[Botond Ballo [:botond]]

I just had an IRC conversation with :dbaron. It should clarify what the
expected behaviour is for viewports. Unfortunately neither our current
behaviour, nor our behaviour with bug 950488 backed out, matches that,
though backing out bug 950488 gets us closer.

[15:41] <botond> dbaron: i'm wondering what you mean, in https://bugzilla.mozilla.org/show_bug.cgi?id=975962#c26, by "except, of course, when viewports are involved"
[15:42] <dbaron> botond, if we have <html style="overflow:hidden">, which normally suppresses the ability to scroll, and we're on mobile, and there's no <meta viewport>, then we still get the mobile viewport panning-and-zooming experience
[15:43] <botond> dbaron: ah. i think that's the case in question in the bug
[15:44] <dbaron> botond, ok, do you want to comment, or should I?
[15:44] <botond> dbaron: i will. would just like to clarify one more thing
[15:44] <botond> dbaron: if you zoom in on this overflow:hidden html element
[15:44] <botond> dbaron: you can then pan in the zoomed-in state?
[15:45] <botond> dbaron: but not pan to bring the hidden areas into view?
[15:45] <dbaron> botond, my expectation is that you'd be able to pan within the logical viewport, but not outside of it
[15:45] <botond> dbaron: ok, got it. thanks!
[15:45] <dbaron> botond, though that brings up the interesting question of what the height of the logical viewport should be
[15:45] <dbaron> botond, I'm not confident that determining it based on the device height is the right thing, though it may well work fine for portrait orientation
[15:46] <dbaron> botond, I'm not sure how we do that now
[15:46] <botond> dbaron: now, if you're not zoomed in, and you're overflow:hidden, you cannot pan at all. is that wrong?
[15:47] <dbaron> botond, that probably seems ok, as long as you can pan when you zoom in
[15:47] <dbaron> botond, though it might actually make sense to allow some vertical panning even when not zoomed in, particularly in landscape
[15:48] <dbaron> botond, depending on how we decide what the height of the logical viewport is
[15:48] <botond> dbaron: how can we know how much of the page is intended to be visible, and how much hidden by the overflow:hidden?
[15:48] *** ChanServ gives channel operator privileges to jet.
[15:48] <dbaron> botond, well, if it's lacking a <meta viewport>, we assume that it was designed for a viewport with a certain width (is it 980px these days?) and a certain height
[15:49] <dbaron> botond, I suspect that we currently determine that height based on the 980px and the device dimensions
[15:49] <dbaron> botond, which is probably ok
[15:50] <dbaron> botond, since the whole logical viewport business (or whatever we call it these days; I'm not sure) is really a hack to make desktop-designed web pages work on mobile
[15:50] <botond> dbaron: so, suppose there is not <meta viewport>, so the viewport is 980 px wide, but our screen is less than 980 px wide
[15:50] <dbaron> botond, so the logic of what I'm saying as that the mobile UI shouldn't allow you to pan to areas that wouldn't be visible on desktop
[15:50] <botond> dbaron: and the html is overflow:hidden
[15:50] <botond> dbaron: you'd expect to still be able to pan horizontally up to 980px?
[15:50] <dbaron> botond, yes
[15:51] <dbaron> botond, since the overflow:hidden would be limiting overflow outside of the {inner,logical,980px,your term here} viewport
[15:51] <dbaron> botond, rather than limiting overflow from the screen

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #30)
> I just had an IRC conversation with :dbaron. It should clarify what the
> expected behaviour is for viewports. Unfortunately neither our current
> behaviour, nor our behaviour with bug 950488 backed out, matches that,
> though backing out bug 950488 gets us closer.

To elaborate on this: with overflow:hidden on the <html> element, and
no <meta viewport> tag with user-scalable=no:

  - The intended behaviour is to be able zoom and pan within
    the logical viewport.

  - The current behaviour is to not be able zoom and pan.

  - The behaviour with bug 950488 backed out is to be able
    to zoom and pan within the region initially visible
    on the screen. This often corresponds to the logical
    viewport, but not always.

Comment 32

[Jason Smith [:jsmith]]

Back over to the nom queue - Botond clarified in IRC & above that we are not doing the right behavior here, so we need to fix this in gecko.

Comment 33

[Jason Smith [:jsmith]]

Mentioned in IRC, but here's what I think we should do:

Prep a patch that fixes bug 950488 correctly with the backout of bug 950488 included in the least risky fashion as possible.

That way, we avoid starting trouble with QC here, as backing out bug 950488 without the fix will reopen a CS blocker. Additionally, we'll get the root cause addressed as well.

To keep regression risk under control, I think we should get the patches ready & then spin a build with those patches for QA exploratory testing against regression risk areas. Then, if everything looks okay, we'll land.

Comment 34

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

Sounds reasonable. I'll try to put something together for this.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I should also note that while I have an understanding of how things (overflow:hidden, viewports) are supposed to work, that understanding could certainly be contradicted by evidence.

Comment 36

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> Sounds reasonable. I'll try to put something together for this.

BenWa and I discussed this today. We think a good place to start would be to look at the attempt for fixing bug 950488 that was posted in https://bugzilla.mozilla.org/show_bug.cgi?id=950488#c19 and see whether it works or can be made to work.

Comment 37

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

I just came up with pretty much that exact patch. Was there a reason that patch wasn't used to begin with?

Comment 38

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

(And yes, it seems to fix the problem on bug 950488, but I need to test some overflow:hidden documents to see how they behave)

Comment 39

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #38)
> (And yes, it seems to fix the problem on bug 950488, but I need to test some
> overflow:hidden documents to see how they behave)

Btw - if it helps, we could get Martijn to spin a build with the patches needed & do some regression testing before landing. We just need to know the exact patches to apply & in what order in a custom build.

Comment 40

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> I just came up with pretty much that exact patch. Was there a reason that
> patch wasn't used to begin with?

I'm afraid I don't remember and there is no record of it at bug 950488. Maybe BenWa remembers.

Comment 41

[Benoit Girard (:BenWa)]

I don't recall exactly but I remember trying that patch. You're liking going to see a few messages bounce back and fort in the fullscreen case and a few screen flashes as the FrameMetrics is being negotiate between the main thread and the compositor. I forget if the steady state had rendering errors or not. Instead of solving this we decided that restricting scrolling and not changing the scroll rect matched better with what overflow:hidden represented and dealt with some ambiguous overscroll cases. The problem is that I hadn't anticipated this case so in insight the fix we landed was wrong.

Comment 42

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

The problem with just that patch is that once you zoom in, you can scroll downwards/rightwards past the bounds of the initial view, and you can never scroll leftwards/upwards.

The reason this happens is because once you zoom in, the mScrollableRect remains the same in CSS pixels, giving you some area to scroll in. However as you scroll, the mScrollableRect's top-left corner follows the scroll position, so that "area you can scroll in" always extends to the right and below the currently-visible area.

I took a step back and realized that fundamentally, when you have overflow:hidden on something, no user input should allow changing the scroll position on that element. Even if you have a document with overflow:hidden on the body, and you allow zooming/panning around the "logical viewport" (as per comment 30 and comment 31), the actual scroll offset shouldn't really be changing - the panning should be entirely on the APZ side and undetectable from content. Luckily the APZ is built to handle exactly this kind of disconnect between the "gecko" scroll position and "visual" scroll position, so this should be doable. This should also solve the problem I described above, because if the scroll offset doesn't change then mScrollableRect won't move, and everything should work just fine.

Comment 43

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

Attachment: Allow panning/zooming around the initial viewport of overflow:hidden documents

And indeed this works beautifully.

I can submit a follow-up patch to back out the rest of the plumbing from bug 950488; that doesn't need to be uplifted.

Comment 44

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

Comment on attachment 8383436 [details] [diff] [review]
Allow panning/zooming around the initial viewport of overflow:hidden documents

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

Actually there might be some problems here. Needs more testing.

Comment 45

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

Attachment: Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents (b2g28)

Ah, I just had the horizontal/vertical checks backwards. Fixed now.

Comment 46

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

Attachment: Part 1 rebased to master

Comment 47

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

(In reply to Jason Smith [:jsmith] from comment #39)
> Btw - if it helps, we could get Martijn to spin a build with the patches
> needed & do some regression testing before landing. We just need to know the
> exact patches to apply & in what order in a custom build.

Sure, that would help. I've attached two copies of the patch to this bug; the first is based on 1.3 code and the second is based on master. Putting together builds for both branches and testing the patch prior to landing would be great. Things that need to be verified specifically:
- this bug
- bug 950488
- behaviour on pages/iframes/divs with overflow:hidden on one/both axes. Try to test on pages with and without user-scalable=no in the meta-viewport tag.
- basic sanity check in Gaia apps, since a lot of them use overflow:hidden. I don't expect much there to be affected though since you can't zoom in them either.

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8383444 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents (b2g28)

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

Won't we clobber the asynchronous scroll offset with the gecko one from time to time in NotifyLayersUpdated?

Comment 49

[Jason Smith [:jsmith]]

Okay. Martijn - Can you look at this when you get into work in your timezone?

Comment 50

[Botond Ballo [:botond]]

Comment on attachment 8383444 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents (b2g28)

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

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +109,5 @@
> +  // to whatever it already is. Note that this will leave the APZ's async scroll
> +  // position out of sync with the gecko scroll position, but APZ can deal with that
> +  // (by design). Note also that when we run into this case, even if both axes
> +  // have overflow:hidden, we want to set aSuccessOut to true, so that the displayport
> +  // follows the async scroll position rather than the gecko scroll position.

Where in the code does the displayport "follow the async scroll position"?

My understanding is that:
  - The displayport we set in APZCCallbackHelper is _relative_
    (to some scroll offset).
  - At some point, layout code interprets the displayport as 
    being relative to some scroll offset that it knows about,
    and paints the resulting region of the layer.

It seems that for this approach to work, layout needs to know to interpret the displayport relative to the async scroll position, not the gecko one. How does it know that?

Comment 51

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

(In reply to Botond Ballo [:botond] from comment #48)
> Won't we clobber the asynchronous scroll offset with the gecko one from time
> to time in NotifyLayersUpdated?

Only if some other code actually sets the scroll offset, and we propagate the mUpdateScrollOffset flag along. In which case we probably want to clobber the async scroll offset anyway.

(In reply to Botond Ballo [:botond] from comment #50)
> Where in the code does the displayport "follow the async scroll position"?

APZCCallbackHelper::MaybeAlignAndClampDisplayPort shifts the displayport to account for the difference between the scroll offset coming in from the APZC and the scroll offset that actually gets set in layout (recall that there are slight differences in the two because of rounding and whatnot). With this patch that difference can end up significantly larger, but the code handles it anyway.

Comment 52

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

Attachment: Part 2 - Remove unneeded plumbing

Follow-up patch to rip out the disableScrollX/Y flags assuming the first patch is good.

Comment 53

[Botond Ballo [:botond]]

Comment on attachment 8383444 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents (b2g28)

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

Note that I tested the patch (specifically on the bug 950488 test case, and on http://people.mozilla.org/~bballo/over.html, which has a viewport which is overflow:hidden in only one dimension) both with and without the changes I mention, and they didn't seem to be of consequence (i.e. it worked either way).

What testcase was misbehaving in comment 44 that led you do conclude that switching x and y this way is necessary?

Other than that, the patch looks good (and it's a very neat approach!). I'm a little concerned about possible assumptions made by layout code about layout's scroll offset corresponding to the asynchronous scroll offset at the time the paint was requested - perhaps Timothy can say whether this is something to worry about or not.

::: layout/base/nsDisplayList.cpp
@@ +628,3 @@
>      }
>      if (scrollableFrame->GetScrollbarStyles().mHorizontal == NS_STYLE_OVERFLOW_HIDDEN) {
> +      contentBounds.y = scrollPosition.y;

I believe we should be adjusting 'y' in the mVertical case and 'x' in the mHorizontal case.

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +116,5 @@
> +    targetScrollPosition.x = geckoScrollPosition.x;
> +  }
> +  if (aFrame->GetScrollbarStyles().mHorizontal == NS_STYLE_OVERFLOW_HIDDEN) {
> +    targetScrollPosition.y = geckoScrollPosition.y;
> +  }

Same here, should be 'y' in the mVertical case and 'x' in the mHorizontal case.

Comment 54

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

Re comment 44: i went into the contacts app, to the "add new contact" screen, and scrolled down. with the bad patch it scrolled past the end of the page and then got stuck in a state where i couldn't scroll back to the top.

Please feel free to steal the patch and fix as necessary if it needs to go in before Thursday.

Comment 55

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

Attachment: Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents

(In reply to Botond Ballo [:botond] from comment #53)
> 
> I believe we should be adjusting 'y' in the mVertical case and 'x' in the
> mHorizontal case.
> 

You're right, I musta been on crack when I looked at this earlier. Updated patch (for master) attached. I compiled it but didn't test it.

Comment 56

[Botond Ballo [:botond]]

Comment on attachment 8384299 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents

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

Looks good! Note that I tested the STR you mentioned in comment 54 with the adjusted patch, and I didn't see any problem. From comment 55 it sounds like you didn't either.

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8384299 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents

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

After looking at the Part 2 patch, I realized this patch actually won't work as-is. Since we are still condition on AllowZoom() in OnScaleBegin() and AllowDoubleTapZoom() in OnDoubleTap(), and both AllowZoom() and AllowDoubleTapZoom() return false if either axis is overflow:hidden, we still won't be able to zoom in on overflow:hidden viewports. We need the removal of these conditions to go into the Part 1 patch.

Comment 58

[Botond Ballo [:botond]]

Comment on attachment 8384299 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents

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

(In reply to Botond Ballo [:botond] from comment #57)
> After looking at the Part 2 patch, I realized this patch actually won't work
> as-is. Since we are still condition on AllowZoom() in OnScaleBegin() and
> AllowDoubleTapZoom() in OnDoubleTap(), and both AllowZoom() and
> AllowDoubleTapZoom() return false if either axis is overflow:hidden, we
> still won't be able to zoom in on overflow:hidden viewports. We need the
> removal of these conditions to go into the Part 1 patch.

Sorry, ignore me. The above is not an issue as we don't actually _set_
the disable-scrolling flag with the Part 1 patch.

Comment 59

[Botond Ballo [:botond]]

Comment on attachment 8383753 [details] [diff] [review]
Part 2 - Remove unneeded plumbing

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

This patch looks good.

BenWa mentioned that it might still be useful for APZ to know whether we are overflow:hidden for future use cases. For example, if we want to support effects such as the blue glow on overflow, APZ needs to be able to distinguish between having scrolled to the end of a scrollable frame, and it being an overflow:hidden frame.

To accomodate such future use cases, should we keep the mDisableScrolling flag (and the infrastructure for setting it) around, and just remove its current uses (in AllowZoom(), AdjustDisplacement(), etc.)? Alternately we can remove it now and add it back later when such future use cases actually arise.

Comment 60

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

FWIW, I think it's much safer to remove it and put it back in when (if) we need it.  The history helps you in the later case, and leaving things around just in case goes stale very quickly and confuses the readers of the code in the meantime...

Comment 61

[Botond Ballo [:botond]]

Comment on attachment 8383753 [details] [diff] [review]
Part 2 - Remove unneeded plumbing

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

(In reply to Milan Sreckovic [:milan] from comment #60)
> FWIW, I think it's much safer to remove it and put it back in when (if) we
> need it.  The history helps you in the later case, and leaving things around
> just in case goes stale very quickly and confuses the readers of the code in
> the meantime...

Sounds reasonable. r=me on Part 2 then (for master).

Comment 62

[Sandip Kamat]

PM Triage: Stays 1.3+

Comment 63

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8384299 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents

Sorry for the delay.

Comment 64

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

Thanks.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c12d4d6aa91c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/62c2807d9680

Comment 65

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

I rebased part 1 to b2g28 but in testing the patch I noticed that clicking on links while zoomed in seemed a little broken. I will test on master to see if it's a bug in the patch, the rebase, or a pre-existing issue.

I also noticed that in the browser, a page can set overflow:hidden and then call scrollTo(0, 100) for example which will lock the browser to y=100 and make it impossible to access the URL bar. I think this is a pre-existing issue as well but we should probably fix it.

Comment 66

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #65)
> I also noticed that in the browser, a page can set overflow:hidden and then
> call scrollTo(0, 100) for example which will lock the browser to y=100 and
> make it impossible to access the URL bar. I think this is a pre-existing
> issue as well but we should probably fix it.

I filed bug 980994 for this issue.

Comment 67

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #65)
> I rebased part 1 to b2g28 but in testing the patch I noticed that clicking
> on links while zoomed in seemed a little broken. I will test on master to
> see if it's a bug in the patch, the rebase, or a pre-existing issue.

So this is a pre-existing issue which is more reliably exposed by the patches on this bug. Turns out the APZC::GetTransformToLastDispatchedPaint is wrong in the cases where the dispatched paint is rejected. This happens in the case of overflow:hidden documents with this patch, but can also happen without this patch if we don't enter the body of the condition at [1]. The difference is that without this patch it's a transient state and we're unlikely to hit it often, whereas with this patch it is reliably reproducible. I will file a new bug to track this issue and make it block this one.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp?rev=3601c5bc51e1#110

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c12d4d6aa91c
https://hg.mozilla.org/mozilla-central/rev/62c2807d9680

Comment 69

[Ryan VanderMeulen [:RyanVM]]

Please request approval-mozilla-b2g28 on this when it is ready for v1.3 uplift.

Comment 70

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

Comment on attachment 8383753 [details] [diff] [review]
Part 2 - Remove unneeded plumbing

I changed my mind about not uplifting this; I think it's better to uplift it to keep things consistent and make future uplifts easier. The rebase isn't too bad, and easy to verify because the compiler yells if you screw up.

Comment 71

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

Attachment: Rollup parts 1-2 for b2g28

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 #): bug 950488
User impact if declined: cannot zoom in on pages that have overflow:hidden set at the top level. hotmail.com (logged in) is one example of such a page
Testing completed: locally, on m-c.
Risk to taking this patch (and alternatives if risky): Medium/high risk. It's a pretty big change and while I think it's the right thing to do, it may have unforseen consequences. There was already one piece of fallout (bug 981029) which will need to be uplifted with this. Not much in the way of alternatives though; I think it's worth uplifting this to fix the problem as it makes some sites unusable.
String or UUID changes made by this patch: none

Comment 72

[Ryan VanderMeulen [:RyanVM]]

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