New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 322223 link

Starred by 5 users

Issue metadata

Status: Verified
Owner:
Closed: Dec 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression



Sign in to add a comment

overflow: hidden on <body> prevents scrolling when zoomed in

Project Member Reported by bokan@chromium.org, Nov 21 2013

Issue description

Version: 33.0.1715.0 (ToT)
OS: Android

What steps will reproduce the problem?
1. Open https://googledrive.com/host/0B5LHF4TU99V4X2V3bUZXdkRtSXc/overflow-x-hidden.html
2. Zoom in on the page
3. Try to scroll the viewport around

The viewport can't be scrolled around - I get the 'blue glow' overscroll effect. 

There seem to be some simmilar bugs such as:
 crbug.com/181315  and  crbug.com/245684  but those seem like they've been broken for a while. This looks like a recent regression - my test page works fine on 31.0.1650.59
 

Comment 1 by klo...@chromium.org, Nov 22 2013

Owner: jdduke@chromium.org
Status: Assigned
Another similar issue, crbug/316135.

Jared, can you help to take a look at this too? Thanks.
Cc: vollick@chromium.org wangxianzhu@chromium.org

Comment 3 by jdduke@chromium.org, Nov 22 2013

Cc: jdduke@chromium.org
Owner: sadrul@chromium.org
sadrul@, I think this is directly related to https://codereview.chromium.org/30793002.   I'm hitting LayerImpl's |!user_scrollable_horizontal_| condition after zooming in and trying to scroll.  I don't even know what the proper behavior here is, but I imagine we should still be able to scroll with a zoomed viewport?

Comment 4 by owale@chromium.org, Nov 26 2013

Issue 323484 has been merged into this issue.

Comment 5 by owale@chromium.org, Nov 26 2013

Labels: M-32 ReleaseBlock-Stable
Also repros in M32 (build 32.0.1700.31).

Real-world examples:
http://toucharcade.com/
http://thebuglepodcast.com/
Cc: aelias@chromium.org tdander...@chromium.org rbyers@chromium.org
I think the desired behaviour is poorly defined for this case. If we do not allow scrolling a body with 'overflow: hidden', then in some sense, we shouldn't allow pinch either, because pinch inevitably involves some amount of scroll. If we do still allow pinch, then the current scrolling behaviour after pinch (the original bug reported in this bug) seems reasonable to me.

Considering that it is possible to scroll an 'overflow: hidden' body in ios, I say we do the same for chrome, and allow scrolling too (both in fast and slow paths).

/cc +rbyers@, aelias@ for opinion.
Cc: wjmaclean@chromium.org
My thinking is that we should allow scrolling within the initial containing block, but not over the entire document.  This behavior should be easier to achieve with the two-scroll-layer pinch model, by marking only the inner viewport as scrollable.
How would the pinch-generated-scroll work in this case (e.g. do we allow body.scrollTop to change during the pinch)?
That question is a more general one for pinch zoom as a whole.  We have the option of reporting either the inner viewport position or outer one. If we report the outer one (safer for backwards compat), then scrollTop would never change with body overflow: hidden.
I agree with #7.  I think of document scrolling (changing the position of the document in the outer viewport) as a completely separate thing from panning (moving the inner pinch-zoom viewport relative to the outer one) - although it's not always obvious to the user which one they're doing.

Marking the body, or any element for that matter, as overflow: hidden should affect the former (scrolling) but not the latter (panning).  A key design goal of pinch-zoom is that it's transparent to the application.  It follows from that that scrollTop would never change as a result of panning alone.

Comment #7 is the correct way to go, and in fact with the current PZ work I think this just happens automatically.
Labels: -Pri-2 Pri-1
Do we have a plan here? I'm encountering a good number of popular desktop-ish sites that are rendered unworkable (on mobile) by this.  If nothing else we should at least revert crrev.com/30793002 for M32 while we work on a more permanent fix.
Sadrul and I discussed this briefly and agreed we should revert crrev.com/30793002 for now until this can be solved correct.  Sadarul, can you do that ASAP please?
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 10 2013

------------------------------------------------------------------------
r239675 | sadrul@chromium.org | 2013-12-10T05:40:09.632951Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host_impl_unittest.cc?r1=239675&r2=239674&pathrev=239675
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/layers/layer_impl_unittest.cc?r1=239675&r2=239674&pathrev=239675
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/layers/layer_impl.cc?r1=239675&r2=239674&pathrev=239675

cc: Allow 'overflow: hidden' layers to be scrolled again.

Disabling scroll of 'overflow: hidden' layers break sites when user zooms in. So allow
scrolling such layers until a better fox for panning in zoomed content is available.

BUG= 322223 
R=aelias@chromium.org

Review URL: https://codereview.chromium.org/109343003
------------------------------------------------------------------------
Labels: Merge-Requested
This is now fixed on ToT. Requesting for merge.

Comment 16 by kareng@google.com, Dec 16 2013

Labels: -Merge-Requested Merge-Approved
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 16 2013

Labels: -Merge-Approved merge-merged-1700
------------------------------------------------------------------------
r240987 | sadrul@chromium.org | 2013-12-16T19:54:43.059595Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/cc/trees/layer_tree_host_impl_unittest.cc?r1=240987&r2=240986&pathrev=240987
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/cc/layers/layer_impl_unittest.cc?r1=240987&r2=240986&pathrev=240987
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/cc/layers/layer_impl.cc?r1=240987&r2=240986&pathrev=240987

Merge 239675 "cc: Allow 'overflow: hidden' layers to be scrolled..."

> cc: Allow 'overflow: hidden' layers to be scrolled again.
> 
> Disabling scroll of 'overflow: hidden' layers break sites when user zooms in. So allow
> scrolling such layers until a better fox for panning in zoomed content is available.
> 
> BUG= 322223 
> R=aelias@chromium.org
> 
> Review URL: https://codereview.chromium.org/109343003

TBR=sadrul@chromium.org

Review URL: https://codereview.chromium.org/115153009
------------------------------------------------------------------------
Status: Fixed
Labels: Verified-M32
Verified fix in M32 - 32.0.1700.94 with steps from #0. Thanks!
Status: Verified
Verified fix in M33 - 33.0.1750.21 with Steps from #0.
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 14 2014

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af915a887e3ec61eb029822ac7577a9198628935

commit af915a887e3ec61eb029822ac7577a9198628935
Author: aelias <aelias@chromium.org>
Date: Tue Oct 14 04:12:04 2014

Respect user_scrollable disabling in pinch-virtual-viewport mode.

user_scrollable_x/y are used to suppress impl-side scrolling when the
page specifies overflow: hidden.  This has been disabled for a while
because of poor interaction with pinch zoom.  In the new pinch-virtual
model, this works better (outer viewport doesn't scroll so that the user
can't see parts of the page not intended to be seen, but pinch zooming
and moving around still works), so re-enable it when that setting is
active.

Note that I'm also planning to start using this for disabling scrolling
for fullscreen elements.

BUG= 322223 , 175502 , 411072 

Review URL: https://codereview.chromium.org/626403003

Cr-Commit-Position: refs/heads/master@{#299428}

[modify] https://chromium.googlesource.com/chromium/src.git/+/af915a887e3ec61eb029822ac7577a9198628935/cc/layers/layer_impl.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/af915a887e3ec61eb029822ac7577a9198628935/cc/layers/layer_impl.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/af915a887e3ec61eb029822ac7577a9198628935/cc/layers/layer_impl_unittest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/af915a887e3ec61eb029822ac7577a9198628935/cc/trees/layer_tree_host_impl.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/af915a887e3ec61eb029822ac7577a9198628935/cc/trees/layer_tree_host_impl_unittest.cc

Sign in to add a comment