829862 - touch panning doesn't scroll the parent when touching inside something with 'overflow'

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Steps to reproduce:
 1. load  http://en.m.wikipedia.org/wiki/Assistant_Secretary_of_the_Navy
 2. scroll down to the section "Assistant Secretaries of the Navy, 1861-1954"
 3. tap the [v] button next to that section heading to show its contents
 4. scroll the page by panning in the contents of the table

Actual results: page doesn't move when trying to pan vertically by touching an area inside the table.  (The table scrolls horizontally, though.)

Expected results: panning vertically should scroll the page when touching the contents of the table just like it does when touching anywhere else.


This bug is present in a B2G build elf-built, from yesterday afternoon.

This bug is NOT present in Fennec Android nightly.

It's also NOT present for scrollwheel scrolling in Firefox desktop.


I'm presuming that the inability to scroll is coming from this style rule that applies to the table (copied out of inspector):
  #content table {
      overflow-y: hidden;
      overflow-x: auto;
      word-break: normal;
      display: block;
  }

My memory (which may be wrong) of the way we normally deal with is kind of thing (mousewheel/touchpad scrolling or touch scrolling) is that we scroll the closest container that is scrollable in the direction of the scrolling gesture; if that container is at its edge, we look for scrollable ancestors that we might be able to scroll.

Whatever the case, though, we're not doing that on B2G, which leaves this page annoyingly unscrollable (unless you happen to notice there's a narrow scrollable area on the left edge).

I'm actually not sure where the relevant code for this lives given all the things we're doing with layers these days, but I've cc:ed some people who I hope do know.

Comment 1

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

I have **no idea whatsoever** whether this sort of thing should be blocking-basecamp, but nominating so that I'm not the sole person deciding that it shouldn't.

Comment 2

[Jason Smith [:jsmith]]

Switching flags per new usage for tracking? + tef? after retiring basecamp?

Comment 3

[Andrew Overholt [:overholt]]

Josh/Shih-Chiang, is this something related to bug 828367?

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I can make a vertical scroll according to the STR in comment 1, with a *very* straight panning. ;)
I noticed that |mDelayPanning| flag is not correctly reset after BSE hand over the gesture detection to APZC. Therefore, you'll need a few trials to perform a successful vertical scrolling.
Need further investigation on the root cause.

Comment 5

[Martijn Wargers (dead)]

This might be a useful testcase for this bug: http://people.mozilla.org/~mwargers/tests/panning/overflowx_scrolling.html

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I think there are two issues been addressed:

1. B2G doesn't implement axis locking (see bug 818492), therefore, BSE will make a horizontal scroll on the subframe if your panning gesture is not straight enough. We can observe this behavior from the test case in comment 5.

2. The state transition in APZC is not correct if a web page has both touch event listener and scrollable subframe. here is the STR:
   1. open http://people.mozilla.org/~schien/test_touch_scroll.html in browser app.
   2. make a horizontal scroll on green section.
   3. tap on blue section and scroll up.

   EXPECTED: vertical scroll is made on the entire page.

   ACTUAL: page is not scrolling until you make the second vertical scroll.

bug is found in this line: http://dxr.mozilla.org/mozilla-central/gfx/layers/ipc/AsyncPanZoomController.cpp.html#l1412

Comment 8

[Jason Smith [:jsmith]]

Renom. This makes marketplace's reviewer tools incredibly hard to use, and reviewers need to do their job.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Should not use mDelayPanning on a touch-enabled page.

We already enqueue all the touch events while we browse a web page with touch listeners. Setting up mDelayPanning flag will confuse APZC about the prior state.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Jason Smith [:jsmith] from comment #8)
> Renom. This makes marketplace's reviewer tools incredibly hard to use, and
> reviewers need to do their job.
Hi jsmith, would you like to try this patch to see if bug 828885 is really a dup of this bug?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 705709 [details] [diff] [review]
Should not use mDelayPanning on a touch-enabled page.

I don't quite understand this patch.  How is the mMayHaveTouchListeners and mDelayPanning state confusing AsyncPanZoomController?

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

here is the event sequence:

1. APZC receives touch-start while mMayHaveTouchListeners = true, enqueue the event
2. BES receives touch-start, fire detect-scrollable-subframe
3. TabChild fire content-received-touch for touch start.
4. APZC receive detect-scrollable-subframe, set mDelayPanning = true
5. APZC receive content-received-touch for touch start, set state to "TOUCHING" because mDelayPanning == true, dequeue touch-start and ignore it since the state is in TOUCHING

Since APZC ignore the touch-start and doesn't set the reference point for panning, the following touch-move events cannot be detected as panning gesture.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

<cjones> schien-laptop, so that bug is a race condition?  if there were a TOUCH_MOVE event in the queue, the code would work?

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't block, you *can* get it scrolling and this is something that can be improved upon in future releases.  If there was a very low risk fix available before tomorrow, we could take it for 1.0.0 even though we'll leave this tracking for 19+ (1.0.1) for now.

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> <cjones> schien-laptop, so that bug is a race condition?  
Not really, the event sequence in comment 12 will always happen when you touch a scrollable subframe on a touch-enabled web page.

> if there were a TOUCH_MOVE event in the queue, the code would work?
If there is any touch move event in the queue before APZC receiving disable-default-pan-zoom, APZC will start doing the scroll over the entire page until disable-default-pan-zoom is received.
It's similar to APZC receive touch move event before detect-scrollable-subframe is received. I think it's more like a general synchronization issue between APZC and BES.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 705709 [details] [diff] [review]
Should not use mDelayPanning on a touch-enabled page.

Hi Shih-Chiang, sorry for the review lag here, I'm quite swamped.  I think ajones can review this (no relation ;) ).

Comment 17

[Doug Sherk (:drs) (inactive)]

Comment on attachment 705709 [details] [diff] [review]
Should not use mDelayPanning on a touch-enabled page.

Reassigning review to me as per talk with ajones on IRC.

Comment 18

[Doug Sherk (:drs) (inactive)]

Comment on attachment 705709 [details] [diff] [review]
Should not use mDelayPanning on a touch-enabled page.

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

I don't think that making mDelayPanning and mMayHaveTouchListeners sort of mutually exclusive is the solution here. 

For starters, mDelayPanning is new to me but I read a bit about it and I think I understand it. I also think I could benefit from debugging this code a bit myself.

Anyways, I think the key points are here:
> 4. APZC receive detect-scrollable-subframe, set mDelayPanning = true
> 5. APZC receive content-received-touch for touch start, set state to "TOUCHING" 
> because mDelayPanning == true, dequeue touch-start and ignore it since the state is 
> in TOUCHING

Notice that we're only currently breaking content-received-touch when detect-scrollable-subframe is fired before it. This patch will break content in situations where we have both touch listeners that are actually messing with touch events and subframes. If the touch listeners aren't mutating the touch events, then yes, this will fix it.

> Since APZC ignore the touch-start and doesn't set the reference point for 
> panning, the following touch-move events cannot be detected as panning gesture.

Why don't we just set the touch reference point when we deque it then, regardless of state?

Comment 19

[Jason Smith [:jsmith]]

Removing needsinfo - reviewer tools test case is no longer valid, given that we implemented a safe workaround. Use the test case from comment 0 for testing the patch.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Alfredo will continue the work of this bug. :)

Comment 21

[Alfredo Yang (:alfredo)]

Attachment: Recalculate scrolling direction while travering to parent node.

The parent node's scrolling direction could be different to child node. When traversing to parent node, the scrolling direction needs to be recalculated.

Comment 22

[Alfredo Yang (:alfredo)]

try server result:
https://tbpl.mozilla.org/?tree=Try&rev=8cc0785f6ce1

Comment 23

[Alfredo Yang (:alfredo)]

Attachment: Recalculate scrolling direction while travering to parent node.

Update checkin comment.
Carry r+ from comment 21.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc08a7472119

Alfredo - we typically use a= in a commit message to indicate approval of a patch for landing on a release branch. Please don't add it there unless that's the case.

Comment 25

[Alfredo Yang (:alfredo)]

Umm, I misunderstand 'a' as 'author', thanks.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc08a7472119