The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla24

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dbaron, Assigned: alfredo)

Tracking

Trunk
mozilla24
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:-, b2g1819+)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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.
blocking-basecamp: --- → ?
Switching flags per new usage for tracking? + tef? after retiring basecamp?
blocking-b2g: --- → tef?
blocking-basecamp: ? → ---
tracking-b2g18: --- → ?
Josh/Shih-Chiang, is this something related to bug 828367?
Flags: needinfo?
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.
Assignee: nobody → schien
Flags: needinfo?
This might be a useful testcase for this bug: http://people.mozilla.org/~mwargers/tests/panning/overflowx_scrolling.html
blocking-b2g: tef? → -
tracking-b2g18: ? → 19+
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

Updated

4 years ago
Duplicate of this bug: 828885
Renom. This makes marketplace's reviewer tools incredibly hard to use, and reviewers need to do their job.
blocking-b2g: - → tef?
Created attachment 705709 [details] [diff] [review]
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.
Attachment #705709 - Flags: review?(jones.chris.g)
(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?
Flags: needinfo?(jsmith)
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?
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.
<cjones> schien-laptop, so that bug is a race condition?  if there were a TOUCH_MOVE event in the queue, the code would work?
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.
blocking-b2g: tef? → -
(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 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 ;) ).
Attachment #705709 - Flags: review?(jones.chris.g) → review?(ajones)
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.
Attachment #705709 - Flags: review?(ajones) → review?(bugzilla)
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?
Attachment #705709 - Flags: review?(bugzilla)
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.
Flags: needinfo?(jsmith)
Alfredo will continue the work of this bug. :)
Assignee: schien → ayang
(Assignee)

Comment 21

4 years ago
Created attachment 754730 [details] [diff] [review]
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.
Attachment #754730 - Flags: review?(21)
Attachment #754730 - Flags: review?(21) → review+
(Assignee)

Comment 22

4 years ago
try server result:
https://tbpl.mozilla.org/?tree=Try&rev=8cc0785f6ce1
(Assignee)

Comment 23

4 years ago
Created attachment 758489 [details] [diff] [review]
Recalculate scrolling direction while travering to parent node.

Update checkin comment.
Carry r+ from comment 21.
Attachment #705709 - Attachment is obsolete: true
Attachment #754730 - Attachment is obsolete: true
Attachment #758489 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
Keywords: checkin-needed
(Assignee)

Comment 25

4 years ago
Umm, I misunderstand 'a' as 'author', thanks.
https://hg.mozilla.org/mozilla-central/rev/dc08a7472119
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.