Closed Bug 975962 Opened 6 years ago Closed 6 years ago

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

Categories

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

28 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla30
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: sync-1, Assigned: kats)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 4 obsolete files)

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:
Attached file qxdm LOG
Can you include a video? I think this might be related, if not a dupe of, bug 964935. A video will confirm that.
blocking-b2g: --- → 1.3?
Component: Gaia::Browser → Panning and Zooming
Flags: needinfo?(sync-1)
Keywords: regression
Product: Firefox OS → Core
Version: unspecified → 28 Branch
See Also: → 964935
Attached image index.png
Can't zoom in and zoom out the page by sliding two fringers.
Flags: needinfo?(sync-1)
What device is this reproducible on, other than Sora?  The graphics team doesn't have access to Sora.
(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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Martijn, would you mind reproducing this on Hamachi, verify the STR and see if you can get a regression range?
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(martijn.martijn)
(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.
Flags: needinfo?(sparsons)
Ok this is is being addressed by jzimbrick.
Flags: needinfo?(sparsons)
QA Contact: jzimbrick
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.
Might have been caused by bug 950488. If not that bug, then maybe bug 958549.
(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.
Blocks: 950488
FWIW - This works fine on FxAndroid with a FxOS user agent spoof.
Kats, is this page wrong, or is our logic from bug 950488 flawed/incomplete? (or is there a third option)
Flags: needinfo?(bugmail.mozilla)
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
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attached patch "Backout" of 950488 (obsolete) — Splinter Review
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).
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?
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.
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.
Keywords: compat
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?
Flags: needinfo?(kdubost)
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".
Flags: needinfo?(kdubost) → needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
(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.)
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.
blocking-b2g: 1.3? → ---
Component: Panning and Zooming → Mobile
Keywords: compat, regression
Product: Core → Tech Evangelism
Version: 28 Branch → unspecified
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.)
(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).
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
(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.
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.
blocking-b2g: --- → 1.3?
Component: Mobile → Panning and Zooming
Keywords: regression
Product: Tech Evangelism → Core
Version: unspecified → 28 Branch
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.
Sounds reasonable. I'll try to put something together for this.
Assignee: nobody → bugmail.mozilla
blocking-b2g: 1.3? → 1.3+
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
I just came up with pretty much that exact patch. Was there a reason that patch wasn't used to begin with?
(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)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
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.
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.
Attachment #8381579 - Attachment is obsolete: true
Attachment #8383436 - Flags: review?(tnikkel)
Attachment #8383436 - Flags: review?(botond)
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.
Attachment #8383436 - Flags: review?(tnikkel)
Attachment #8383436 - Flags: review?(botond)
Ah, I just had the horizontal/vertical checks backwards. Fixed now.
Attachment #8383436 - Attachment is obsolete: true
Attachment #8383444 - Flags: review?(tnikkel)
Attachment #8383444 - Flags: review?(botond)
(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.
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(jsmith)
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?
Okay. Martijn - Can you look at this when you get into work in your timezone?
Flags: needinfo?(jsmith)
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?
(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.
Follow-up patch to rip out the disableScrollX/Y flags assuming the first patch is good.
Attachment #8383753 - Flags: review?(botond)
Attachment #8383444 - Attachment description: Allow panning/zooming around the initial viewport of overflow:hidden documents → Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents (b2g28)
Attachment #8383445 - Attachment description: Patch rebased to master → Part 1 rebased to master
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.
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.
(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.
Attachment #8383444 - Attachment is obsolete: true
Attachment #8383445 - Attachment is obsolete: true
Attachment #8383444 - Flags: review?(tnikkel)
Attachment #8383444 - Flags: review?(botond)
Attachment #8384299 - Flags: review?(tnikkel)
Attachment #8384299 - Flags: review?(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.
Attachment #8384299 - Flags: review?(botond) → review+
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.
Attachment #8384299 - Flags: review+ → review-
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.
Attachment #8384299 - Flags: review- → review+
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.
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 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).
Attachment #8383753 - Flags: review?(botond) → review+
PM Triage: Stays 1.3+
Comment on attachment 8384299 [details] [diff] [review]
Part 1 - Allow panning/zooming around the initial viewport of overflow:hidden documents

Sorry for the delay.
Attachment #8384299 - Flags: review?(tnikkel) → review+
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
https://hg.mozilla.org/mozilla-central/rev/c12d4d6aa91c
https://hg.mozilla.org/mozilla-central/rev/62c2807d9680
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Please request approval-mozilla-b2g28 on this when it is ready for v1.3 uplift.
Flags: needinfo?(martijn.martijn)
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.
Attachment #8383753 - Attachment description: Part 2 - Remove unneeded plumbing (patch for master only; do not uplift) → Part 2 - Remove unneeded plumbing
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
Attachment #8390817 - Flags: approval-mozilla-b2g28?
Attachment #8390817 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Depends on: 984548
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.