Closed Bug 805638 Opened 12 years ago Closed 12 years ago

Nested scrollable regions doesn't work properly

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: sicking, Assigned: schien)

References

Details

Attachments

(1 file, 3 obsolete files)

find parent scrollable, v3
4.78 KB, patch
schien
: review+
Details | Diff | Splinter Review 
We appear to have problems with nested scrollable regions.

If a page is scrollable and contains a scrollable region and the user puts down the finger in the scrollable region he can *only* scroll that scrollable region.

This happens even if the scrollable region can only scroll up/down whereas the user is trying to scroll the page left/right.

It's often very unclear that part of the page is scrollable, which means that it can be very unclear why the page isn't scrolling when you move around the finger.

What we *should* do IMHO is if you put your finger in a scrollable region which is scrolled all the way to the right, and then move your finger further to the right, we should scroll any parent scrollable region which isn't scrolled all the way to the right. Until that region reaches the right-most edge at which point we'd scroll the next parent and so on.

This made gmail completely unusable to me until I found some pixels on the edge which weren't scrollable and which let me scroll the page itself.


The above is similar to how we handle mousewheel scrolling on desktop firefox. If you have a scrollable region in a scrollable page and put the mouse pointer over the scrollable page and then use the mousewheel we will scroll the scrollable region if it's not scrolled all the way in the direction that the user is trying to scroll. If the scrollable region is already at the edge we scroll the page, or whatever parent scrollable region exists.


Ideally momentum scrolling shouldn't automatically switch to scrolling a parent region once we reach the end though. The user should be required to do another swipe with the finger to scroll the parent. Again, this is very similar to mousewheel handling on desktop.
blocking-basecamp: ? → +
I assume it's not AsyncPanZoomController doing this, since it doesn't handle nested scrolled regions.

Where does B2G's synchronous panning code live?
This is a regression.  Test case is

http://people.mozilla.com/~cjones/scrolling.html

which has an topmost scrollable region that apzc manages, along with a scrollable div and iframe.  The inner scrollable frames worked previously with apzc.  And indeed, I see pan gestures in those regions being squelched, so the code is at least partially working still.

Sync fallback code is in

http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementScrolling.js
I meant to say, that particular test case used to work with apzc.  It's what drs and I tested with.
I think what Jonas means is that we can only scroll an inner scrollable region even if the inner scrollable region cannot scroll any more. I also observe the same usability issue in scroll bar tests (within UI Test app).
We only set the |scrollLeft| and |scrollTop| to the inner scrollable node, but not propagate to its scrollable parent node. We can solve this bug by changing the target scrollable region to its parent while we reach the end of a scrollable region.

scroll callback code is in
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementScrolling.js#170
This is a proof of concept for continuous scrolling.
Assignee: nobody → schien
Attachment #677327 - Flags: feedback?(jones.chris.g)
Attachment #677327 - Flags: feedback?(jones.chris.g) → feedback?(21)
Comment on attachment 677327 [details] [diff] [review]
WIP - change scrolling node to parent while reaching the end of scrolling

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

I think you're going the right way.

::: dom/browser-element/BrowserElementScrolling.js
@@ +175,5 @@
>  
>      return [null, null];
>    },
>  
> +  _findPannable: function cp_findPannable(node) {

Instead of duplicating all the logic of getPannable, you may want to create a helper to share this code.

@@ +226,5 @@
> +          let parentScrollable = ContentPanning._findPannable(content.parentNode);
> +          if (parentScrollable) {
> +            content = parentScrollable;
> +            return scroll(delta);
> +          }

You may want to move this code outside the generated function so you won't need to search for the parent on every move if it is needed.
Attachment #677327 - Flags: feedback?(21) → feedback+
Attached patch find parent scrollable, v1 (obsolete) — — Splinter Review 
update patch according to comment 6.

> You may want to move this code outside the generated function so you won't need to search for the parent on every move if it is needed.
I cannot find a correct way to move these if statement outside the generated function, since we need to find grandparent scrollable node if the parent node also reaches the end.
Attachment #677327 - Attachment is obsolete: true
Attachment #679002 - Flags: review?(21)
Comment on attachment 679002 [details] [diff] [review]
find parent scrollable, v1

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +165,5 @@
>        node = node.parentNode;
>      }
>  
>      if (ContentPanning._asyncPanZoomForViewportFrame &&
> +        nodeContent === content) {

Please keep the comment.

@@ +204,1 @@
>      }

It would be nice if this function can be as light as possible and do not have any allocation (let) inside it. Can you rewrite it in such a way?

Also it seems like your patch will work into one direction only. Let's say you scroll to the right of the inner child and reach the end, then you will start to scroll the parent container. Keeping your finger pressed you then start to scroll to the left and reach the end of the parent container you won't scroll the chil anymore. r- because of this and the additional allocations.
Attachment #679002 - Flags: review?(21) → review-
Try run for bb849d289729 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bb849d289729
Results (out of 17 total builds):
    success: 16
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-bb849d289729
(In reply to Vivien Nicolas (:vingtetun) from comment #8)
> Comment on attachment 679002 [details] [diff] [review]
> find parent scrollable, v1
> 
> Review of attachment 679002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +165,5 @@
> >        node = node.parentNode;
> >      }
> >  
> >      if (ContentPanning._asyncPanZoomForViewportFrame &&
> > +        nodeContent === content) {
> 
> Please keep the comment.
> 
> @@ +204,1 @@
> >      }
> 
> It would be nice if this function can be as light as possible and do not
> have any allocation (let) inside it. Can you rewrite it in such a way?
I'll do my best to try removing the allocation. :)
> 
> Also it seems like your patch will work into one direction only. Let's say
> you scroll to the right of the inner child and reach the end, then you will
> start to scroll the parent container. Keeping your finger pressed you then
> start to scroll to the left and reach the end of the parent container you
> won't scroll the chil anymore. r- because of this and the additional
> allocations.
Actually in this patch, we only change scrolling target if the inner child reach the end at the first place. That is, if we are able to scroll the inner child, the scrolling target will stay on inner child during the entire panning action. That's why I use |firstScrolling| and I'll put a comment describing this behavior.
Comment on attachment 679002 [details] [diff] [review]
find parent scrollable, v1

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

I notice a logic error in the v1 patch, will fix it in v2.

::: dom/browser-element/BrowserElementScrolling.js
@@ +193,3 @@
>        }
> +
> +      if (firstScroll && isScrolling) {

the expression should be |firstScroll && !isScrolling|
Try run for bb849d289729 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bb849d289729
Results (out of 18 total builds):
    success: 16
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-bb849d289729
Try run for bb849d289729 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bb849d289729
Results (out of 19 total builds):
    success: 17
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-bb849d289729
Attached patch find parent scrollable, v2 (obsolete) — — Splinter Review 
update patch according to previous comment. Try using iteration instead of recursion and reduce the allocation in the generated scroll callback function.
Attachment #679002 - Attachment is obsolete: true
Attachment #679592 - Flags: review?(21)
Comment on attachment 679592 [details] [diff] [review]
find parent scrollable, v2

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

::: dom/browser-element/BrowserElementScrolling.js
@@ +202,3 @@
>      function scroll(delta) {
> +      for (target = content; target;
> +          target = ContentPanning._findPannable(target.parentNode)) {

I'm really worried that you call findPannable in the scroll path. Have you checked that it does not regress fps? I believe this create a lot of allocations on the stack as well as reflow. Have you tried your patch on the device?
(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> Comment on attachment 679592 [details] [diff] [review]
> find parent scrollable, v2
> 
> Review of attachment 679592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +202,3 @@
> >      function scroll(delta) {
> > +      for (target = content; target;
> > +          target = ContentPanning._findPannable(target.parentNode)) {
> 
> I'm really worried that you call findPannable in the scroll path. Have you
> checked that it does not regress fps? I believe this create a lot of
> allocations on the stack as well as reflow. Have you tried your patch on the
> device?
Yep, I've test this patch on both unagi and otoro.
(In reply to Shih-Chiang Chien [:schien] from comment #16)
> (In reply to Vivien Nicolas (:vingtetun) from comment #15)
> > Comment on attachment 679592 [details] [diff] [review]
> > find parent scrollable, v2
> > 
> > Review of attachment 679592 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/browser-element/BrowserElementScrolling.js
> > @@ +202,3 @@
> > >      function scroll(delta) {
> > > +      for (target = content; target;
> > > +          target = ContentPanning._findPannable(target.parentNode)) {
> > 
> > I'm really worried that you call findPannable in the scroll path. Have you
> > checked that it does not regress fps? I believe this create a lot of
> > allocations on the stack as well as reflow. Have you tried your patch on the
> > device?
> Yep, I've test this patch on both unagi and otoro.
Sorry, I mis-click the "save changes" button...
Actually, |findPannable| will be called only if scrolling cannot be done at the first place, i.e., no reflow at that time. Once any region starts scrolling, we'll not invoke findPannable again. Therefore, this patch should not regress fps.
Here is the test result on unagi. 
(panning up and down in gallery app)
------------ composition -- drawing
  no patch:     42~48        41~46
with patch:     42~47        40~46

(swiping in gallery app)
  no patch:     52~58        51~57
with patch:     53~58        50~57
Comment on attachment 679592 [details] [diff] [review]
find parent scrollable, v2

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

r+ with the |let| moved out of the doScroll function.

::: dom/browser-element/BrowserElementScrolling.js
@@ +185,5 @@
> +    let isScrolling = false;
> +
> +    function doScroll(node, delta) {
> +      if (node instanceof Ci.nsIDOMHTMLElement) {
> +        let oldX = node.scrollLeft, oldY = node.scrollTop;

Can you move all the |let| out of this function to not allocate once per scroll.
Attachment #679592 - Flags: review?(21) → review+
move |let| outside the doScroll(), carry r+ according to comment 18.
Attachment #679592 - Attachment is obsolete: true
Attachment #685432 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0e44ca36bba8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Try run for 142ae63a9841 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=142ae63a9841
Results (out of 257 total builds):
    success: 243
    warnings: 13
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-142ae63a9841
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: