Closed Bug 797411 Opened 12 years ago Closed 12 years ago

overflow: hidden on <body> or <html> doesn't prevent screen-drags from scrolling, in B2G

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: vingtetun, Assigned: dholbert)

References

Details

Attachments

(8 files, 3 obsolete files)

Testcase
325 bytes, text/html
Details
Example of what happens in the First Run Experience
20.62 KB, image/png
Details
testcase 2: "overflow:hidden" on <body>
365 bytes, text/html
Details
testcase 3: "overflow:hidden" on <html>
386 bytes, text/html
Details
reference
430 bytes, text/html
Details
backtrace of scroll event
8.21 KB, text/plain
Details
testcase that we shouldn't break, or ideally allow to break the Browser app: scripted document-scrolling w/ overflow:hidden
415 bytes, text/html
Details
fix v2
2.96 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
Attached file Testcase — 
If a div is positionned out of the viewport with |transform: translateX(100%)| and the css contain a rule like |body { overflow: hidden }| then the scrollbars are hidden but it is possible to scroll the content using the keyboard keys or the finger in the case of Gaia.

See the attached testcase.

This bug affects pretty badly the First-Run-Experience and the Settings application of Gaia but I bet there is other occurrences hidden somewhere since Gaia heavily use the transform property. blocking-basecamp?
Whiteboard: DUPEME
Vivien, is there a workaround that we could use to avoid this?
Whiteboard: DUPEME → DUPEME [blocked-on-input Vivien Nicolas]
(In reply to Andrew Overholt [:overholt] from comment #2)
> Vivien, is there a workaround that we could use to avoid this?

This is doable to workaround that for Gaia Apps only but it is not so easy for other apps that come from third party apps.
Whiteboard: DUPEME [blocked-on-input Vivien Nicolas] → DUPEME
I don't think this is a blocker. This sounds like a platform bug that we've had forever and we've been able to ship both Firefox desktop and Firefox for android without it fixed.

I'd love to see a fix since people do use transforms a lot on mobile, but I wouldn't say this is a blocker.

Jet: Would you be able to find someone on the layout team that could look at this? I suspect this is somewhat important for mobile in general.
blocking-basecamp: ? → -
Summary: overflow: hidden is ignored when a transformed div is positionned out of the viewport → overflow: hidden is ignored when a transformed div is positioned out of the viewport
Blocks: 809618
Blocks: 817181
As bug 805638 just landed, this bug has got worse for Gaia and we don’t have any workaround for bug 817181 any more.

Note that the workaround we used costs about 150ms to the Settings app before the first paint.

Re-nominating for basecamp-blocking.
blocking-basecamp: - → ?
Flags: needinfo?(bugs)
hmm, in fact Tim found a new workaround for this, see his patch for bug 817181, so I'm not rushing to fix this bug any more.
(In reply to Fabien Cazenave [:kaze] from comment #7)
> hmm, in fact Tim found a new workaround for this, see his patch for bug
> 817181, so I'm not rushing to fix this bug any more.

Okay, I'm going to bb- it but feel free to re-nom if the workaround doesn't, um, work.  Thanks.
blocking-basecamp: ? → -
Re-nominating as it turns out the workaround brings a lot of new issues, sorry.
blocking-basecamp: - → ?
FTR, bug 818056 is a likely consequence of the workaround we used to avoid bug 797411.
Blocks: 818056
I know it's late in the game to get that fixed, but gaia devs lost hours trying to find workarounds while not getting flickering etc. Do we have someone that could take a shot at the issue there?
Removing the needinfo flag so that this shows up in triage.

Jet: Do you think that we could find someone from the layout team to at least investigate if this is something that could be backported? We could probably land such a backport after the branchpoint, but we still need it to be a pretty safe patch of course.
Flags: needinfo?(bugs)
I spoke with Jet on IRC. He's going to find an owner for this bug.
Assignee: nobody → bugs
To dholbert for a look...
Assignee: bugs → dholbert
There's nothing transform-specific about this bug. Updating summary (and posting further-reduced testcases in a minute).
Summary: overflow: hidden is ignored when a transformed div is positioned out of the viewport → overflow: hidden on <body> or <html> doesn't prevent arrow-keys / pagedown / spacebar from scrolling
The bug here is that "overflow:hidden" on <body> & <html> will hide the scrollbar, but doesn't stop you from scrolling via keyboard/spacebar/pagedown.

Here's a testcase w/ "overflow:hidden" on the <body> node (and with overflowing content -- a red div sticking off the bottom, which shouldn't be visible or user-scrollable)
...and here's a testcase w/ "overflow:hidden" on the <html> element. (still scrollable w/ keyboard, but shouldn't be)
Attached file reference — 
Here's a reference case for those last two testcases, with "overflow:hidden" on a div just inside the <body> node.  This one refuses to scroll, no matter how hard I try. (short of manually tweaking "scrollTop")
Here's the backtrace of where we create the AsyncScroll event, to do the scrolling.
bug 295020 and bug 325942 are older related bugs.
So basically, what happens is we've got:

  _______________
 |...............|  <--- nsCanvasFrame (canvas frame for root node)
 |.             .|
 |._____________.|
  .             .
  ...............   <--- nsBlockFrame (for the <html> node)

- Various keys appear to be hard-wired to send "line-previous" / "line-next" scroll commands.
- The nsCanvasFrame has a relatively short height, but it's got a _scrollable_ overflow area that includes all of the offscreen content.
- So, when we create the AsyncScroll event, we allow ourselves to scroll, because we've got a large scrollable overflow area. 


QUESTION: Does the gaia client code here (or something that sets it up) have chrome privileges? If so, it can use setScrollPositionClampingScrollPortSize(), a relatively new function (from bug 732016) which is exposed to script via nsIDOMWindowUtils:
  https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1258

That method lets you clamp the scrollable region to the screen-size, preventing that offscreen content from being scrollable.

If we can use that, it'd be a reasonably targeted workaround that would avoid having to make platform changes on a stable branch.
>   <--- nsCanvasFrame (canvas frame for root node)
[...]
>   <--- nsBlockFrame (for the <html> node)

(er, I should clarify that -- the root node _is_ the <html> node, so I should've just said "<html> node" in both labels there. (The <html> node gets a few frames.) Sorry for that mistake.)
No *apps* have chrome-js access. However the platform does of course.

I don't fully understand what the setScrollPositionClampingScrollPortSize function does. What would happen if we called that on every window that is opened? Would it mean that the viewport is never scrollable? What is the reason that we don't call that function for all windows opened in firefox (the answer to that is probably obvious once you understand what the function does).
tn can answer that better than I can, but from glancing at it, it looks like it lets you prevent offscreen content from being scrolled. (outside of a certain configurable size, which in this case would be the viewport-size) If we called it everywhere w/ the viewport-size then it'd prevent you from e.g. scrolling to the bottom of this bugzilla page.  But for apps that are intended to be fullscreen (and no bigger, and not have any scrollable content), it might be appropriate.

Anyway -- if apps can't selectively call setScrollPositionClampingScrollPortSize themselves, then it's probably not the right solution, so never mind about that, probably...
Note that "overflow:hidden" has the same effect regardless of whether it's on <body> or on <html> -- in both cases, it's applied the toplevel scroll-frame -- because we have a hardcoded check to do that, added in bug 234851. (see bug 234851 comment 28)

Code snippet where this happens, for <body> at least:
> 4301   if (aElement->IsHTML(nsGkAtoms::body)) {
> 4302     propagatedScrollToViewport =
> 4303       PropagateScrollToViewport() == aElement;
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4301

...which calls into:

> 2194 /**
> 2195  * This checks the root element and the HTML BODY, if any, for an "overflow" property
> 2196  * that should be applied to the viewport.
[...]
> 2203 nsCSSFrameConstructor::PropagateScrollToViewport()
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#2194
(In reply to Daniel Holbert [:dholbert] from comment #24)
> tn can answer that better than I can, but from glancing at it, it looks like
> it lets you prevent offscreen content from being scrolled. (outside of a
> certain configurable size, which in this case would be the viewport-size) If
> we called it everywhere w/ the viewport-size then it'd prevent you from e.g.
> scrolling to the bottom of this bugzilla page.  But for apps that are
> intended to be fullscreen (and no bigger, and not have any scrollable
> content), it might be appropriate.

The original purpose was the opposite of that: if you are zoomed in on a document in Fennec to allow you to scroll over to the right of the page. The size of the scroll frame hasn't changed but whats actually visible on screen is a subset of the width of the scrollframe so we added this to allow setting the scroll position to where is would not normally be allowed. Not saying it wouldn't work for the other case though.
Ah, ok.

Staying on the "ways to hack around this in a targeted way without landing possibly-scary platform changes on stable branches" kick for a moment:

Is there a reason why we can't fix the apps that suffer from this by just adding an additional wrapper-div, immediately inside their <body>, and making _that_ node be overflow:hidden instead of the body? (like I did in my attached "reference" file -- it works there, at least)
Kaze: Is the workaround in comment 27 what you guys tried and which was just too much of a pain? Or would that workaround be an option?
Flags: needinfo?(kaze)
Gaia team will look at using the proposed workaround. We'd still like to investigate a Platform-level fix, and ship it later, if the code is deemed risky.
I’ll test this in the next 24h: several workarounds are currently piled in the Settings app because of this bug, I need some time to check this out properly — and I’m rushing on another critical bug atm.
Flags: needinfo?(kaze)
(In reply to Fabien Cazenave [:kaze] from comment #30)
> I’ll test this in the next 24h: several workarounds are currently piled in
> the Settings app because of this bug, I need some time to check this out
> properly — and I’m rushing on another critical bug atm.

Resetting needinfo until we get the info :)
Flags: needinfo?(kaze)
kaze, any updates on this?  Does my suggested wrapper-div workaround work?

To be clear, I'm not actively working on this at the moment, since (assuming that it's reasonably easy to work around) it doesn't actually block gaia work, and it'd be somewhat risky to backport a platform-fix onto branches. (It's feasible that a platform fix for this could cause regressions that unexpectedly break scrolling in some edge cases, and that'd be a dataloss / can't-reach-the-button kind of issue, which would be bad on a stable branch.)

If the workaround doesn't work for some reason, then of course that risk/reward calculus would change.
Daniel, *if* we can find a low-risk fix in the platform I would much prefer that.

The gaia team is by far the most overloaded team for B2G right now and so any help we can give them means that we'll have time to fix more blockers before we ship v1.

Especially when it "only" involves fixing things that are real platform bugs anyway. Though of course I realize that a safe fix might mean doing some ugly workarounds that we'd prefer not doing.
OK -- after a bit more digging here, I'm pretty sure b2g/gaia's not directly hitting the gecko bug that the attached testcase reveals -- and in fact, it's sort of a bug in BrowserElementScrolling.js, if anything.

The desktop-browser testcase here is arguably a real gecko bug (covered by the bug numbers in comment 20), where downarrow (which maps to "PresShell::ScrollLine()") and pagedown/space (which maps to ::ScrollPage()) should perhaps be neutered if <html> and/or <body> have overflow:hidden.

But even if we fixed that platform bug, we'd still need to make sure the document was scrollable by script, via the method window.scrollBy(), aka nsGlobalWindow::ScrollBy().  And unfortunately, that precisely the method that gaia calls for touch-based scrolling of apps.

Put another way: from gecko's perspective, user tapping-and-dragging is indistinguishable from scripted page-scrolling, so I don't think we can directly neuter it...
In a little more detail: when an app that suffers from this bug is scrolled, the scroll is performed by nsGlobalWindow::ScrollBy(). Up one level from that is  NS_InvokeByIndex_P. So, we're getting called by script. DumpJSStack() gives this JS stack:

0 doScroll() ["chrome://global/content/BrowserElementChild.js":1103]
    <failed to get 'this' value>
1 scroll() ["chrome://global/content/BrowserElementChild.js":1112]
    this = [object Object]
2 cp_onTouchMove() ["chrome://global/content/BrowserElementChild.js":990]
    this = [object Object]
3 cp_handleEvent() ["chrome://global/content/BrowserElementChild.js":862]
    this = [object Object]

From looking at BrowserElementChild.js, it looks like the interesting lines here actually live in BrowserElementScrolling.js (and are #included into BrowserElementChild.js).  In particular, doScroll()'s call to scrollBy() is here:
> 188     function doScroll(node, delta) {
[...]
> 196         oldX = node.scrollX, oldY = node.scrollY;
> 197         node.scrollBy(delta.x, delta.y);
https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementScrolling.js#188

So: this is a JS-initiated scroll of the document, which gecko is correctly honoring.

To fix this, I think we need to add some checks to the JS there, before we call scrollBy, to determine whether the target node is overflow:hidden before we allow the user to scroll it, or something like that.

Bumping to Core|DOM (where BrowserElementScrolling.js lives), and adding dependency on bug 717844 (which added the original version of the chunk of code in question), and clarifying bug title to be about B2G touch-scrolling.
Component: Layout → DOM
Depends on: 717844
Summary: overflow: hidden on <body> or <html> doesn't prevent arrow-keys / pagedown / spacebar from scrolling → overflow: hidden on <body> or <html> doesn't prevent screen-drags from scrolling, in B2G
Whiteboard: DUPEME
(In reply to Daniel Holbert [:dholbert] from comment #32)
> kaze, any updates on this?  Does my suggested wrapper-div workaround work?

FWIW -- I confirmed that the wrapper-div workaround does work, at least in the B2G desktop client.  (I downloaded the attached "reference" (attachment 691594 [details]), which includes the workaround, and copyied it into e.g. gaia/apps/calculator/index.html, and I verified that it's not scrollable. Whereas if I repeat the steps with one of the attached testcases, it ends up being scrollable).

--> clearing Needinfo
Flags: needinfo?(kaze)
Attached patch wip fix (obsolete) — — Splinter Review 
Here's a WIP fix. It doesn't get the right window-object at the moment, so the getComputedStyle calls end up failing (throwing, actually).  But if that were fixed, I'm pretty sure something along these lines should fix this bug.
(In reply to Daniel Holbert [:dholbert] from comment #36)
> (In reply to Daniel Holbert [:dholbert] from comment #32)
> > kaze, any updates on this?  Does my suggested wrapper-div workaround work?
> 
> FWIW -- I confirmed that the wrapper-div workaround does work, at least in
> the B2G desktop client.

Vivien, can you comment on the suitability of this for B2G device builds?  Are you satisfied with this workaround for v1?
Flags: needinfo?(21)
Well, it actually looks like we have a real fix. In attachment 694737 [details] [diff] [review]. If we can do that that sounds even better!
Attached patch fix v1 (obsolete) — — Splinter Review 
OK, here's an actually-working version of the WIP.

Caveat: I'm not sure whether this breaks zooming+panning on a page with <body style="overflow:hidden">  (e.g. if you want to zoom in on a chunk of text, and then pan across it, but not outside of the <body> viewport).  I'm going to test that now.
Attachment #694737 - Attachment is obsolete: true
So, I ran across one problem with fixing this -- it lets web content very easily hide the browser URLbar / menu / tabstrip-button.

In the browser app, now web pages with <body style="overflow:hidden"> won't be user-scrollable, but they can still scroll themselves with scripting (as intended).  WHICH MEANS: If a web page scrolls itself with a "scrollBy" call, the URL bar and menu button become user-inaccessible. (The user has to scroll the page in order to access them -- but we're not allowing the user to scroll the page -- so they're completely inaccessible.)  The only way for the user to make their browser usable again is to kill it and restart it.

I bet there are already plenty of other ways for web pages to do this, though... e.g. just continuously calling "scrollBy" in a loop (with setTimeouts to yield) would effectively have the same effect.  So, this fix adds a footgun, but maybe it's no worse than already-available footguns.
Attached patch fix v1a (obsolete) — — Splinter Review 
Actually: comment 41 applies to the B2G desktop client, but not to the browser on actual devices -- they use asynchronous zooming+panning, and IIUC the chunk of BrowserElementScrolling.js that "fix v1" touches is only for synchronous panning.

So: The attached patch doesn't fix this bug in the Browser app (w/ async panning&zooming), but that's kind of good, because:
 (a) it means we don't provide the footgun in comment 41 to web pages in the browser, and
 (b) it also means this doesn't break zooming+panning in the browser.

This also doesn't break zooming+panning in apps, because we don't support zooming+panning in synchronously-scrollable content.

So, I'm no longer worried about the concerns I brought up at the end of comment 40 & in comment 41.  Reposting patch w/ a comment tweaked, & requesting review.
Attachment #695004 - Attachment is obsolete: true
Attachment #695086 - Flags: review?(21)
Comment on attachment 695086 [details] [diff] [review]
fix v1a

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +286,5 @@
>          newX = node.scrollLeft, newY = node.scrollTop;
>          return (newX != oldX || newY != oldY);
> +      } else if (node instanceof Ci.nsIDOMWindow) {
> +        let win = node;
> +        let doc = win.document;

Variables declarations has been moved out of the doScroll function to prevent GC. Let's do the same here.
Attachment #695086 - Flags: review?(21)
Flags: needinfo?(21)
Attached patch fix v2 — — Splinter Review 
OK -- I moved the variable-declarations up with all the other ones.
Attachment #695086 - Attachment is obsolete: true
Attachment #695776 - Flags: review?(21)
Comment on attachment 695776 [details] [diff] [review]
fix v2

Seems like this won't cause GC but let's follow what has already been done.
Attachment #695776 - Flags: review?(21) → review+
blocking-basecamp: ? → +
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/d092140b20df
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
I guess getComputedStyle() requires a layout flush equivalent to .scrollLeft/Top?
At most, it requires a style flush -- not a layout flush (no reflows).  Pretty sure that happens here:
 https://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#294

See https://mxr.mozilla.org/mozilla-central/source/content/base/public/mozFlushType.h#20 for the various types of flushes.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: