[Browser] can't scroll in scrollable iframe

VERIFIED FIXED in Firefox 19

Status

()

P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: nhirata, Assigned: schien)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: testrun 2)

Attachments

(1 attachment, 1 obsolete attachment)

1. go to http://www.w3schools.com/tags/att_iframe_scrolling.asp
2. select "try it yourself"
3. go to the first iframe where the scrollbars are shown
4. try to pan in the iframe

Expected: iframe moves
Actual : nothing moves in the iframe.
Perhaps bug 817859 is related to this bug?
Never mind, bug 817859 is another cause.
Component: Gaia::Browser → DOM: Core & HTML
Product: Boot2Gecko → Core
QA Contact: nhirata.bugzilla
blocking-basecamp: ? → +
Shih-Chiang: Could this be a regression from bug 805638?
Assignee: nobody → schien
It's a regression bug existed before bug 805638 was checked in. In browser app, synthetic mouse event will never be sent due to APZC is applied and BrowserElementScrolling depends on synthetic mouse event to recognize scrolling gesture.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1377
I found this bug can also be resolved by the patch I created for bug 814252.
This sounds vaguely like an issue we had on android with the NYT site.
I think the NYT issue you're referring to is bug 750839 which was mostly NYT-specific and an evangelism issue.
Shin-Chiang, do you still think that the fix for bug 814252 is a fix for this?  It sounds like you have a WIP for that other bug - do you have an idea as to when you'd get it to be ready for a review?
Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.
Target Milestone: --- → B2G C3 (12dec-1jan)
I'm pretty sure that fix for bug 814252 is a fix for this one, by comparing to the behavior on a phone with and without my patch. I'll provide a patch for review before 12/10, but I need to land bug 815943 first since it blocks bug 814252.
I also agree with Kartikaya that NYT issue is site-specific.
Priority: -- → P1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 814252
reopen this bug since the latest patch for bug 814252 will not be able to solve this bug.

As my observation in comment 4, we will need to synthesize mouse event even if APZC is enabled. I think this bug represents a more general problem that we don't deliver mouse move event in browser app.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Another way to solve this bug is to allow BrowserElementScrolling handling touch event when APZC is enabled. Scrolling detection will not block any mouse event handler since there is no synthesized mouse event when APZC is enabled.
That would require a more complicated version of attachment 694726 [details] [diff] [review] from bug 823619.  But we can keep that in mind.
Created attachment 695595 [details] [diff] [review]
use touch event for scrolling when APZC is enabled

use touch event for scrolling if APZC is enabled.
Attachment #695595 - Flags: review?(jones.chris.g)
Comment on attachment 695595 [details] [diff] [review]
use touch event for scrolling when APZC is enabled

We can take this while we wait on bug 823619.

>diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js
>--- a/dom/browser-element/BrowserElementScrolling.js
>+++ b/dom/browser-element/BrowserElementScrolling.js
>@@ -1,117 +1,197 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> const ContentPanning = {
>   init: function cp_init() {
>-    ['mousedown', 'mouseup', 'mousemove'].forEach(function(type) {
>-      addEventListener(type, ContentPanning, false);
>-    });
>+    // mouse event will not be synthesized while APZC is enabled

That's not true; mouse events are synthesized on taps when content
doesn't preventDefault() the touchstart.  That's why we're still
filtering them :).

>+  evtFilter: '',

After having worked with this patch for a while, I'm finding this name
confusing.  "Filter" usually means "remove", but here we're using it
to mean "events of this type are not filtered".

Let's rename this to |watchedEventsType|.

>+  findFirstTouch: function cp_findFirstTouch(touches) {
>+    if (!('trackingId' in this)) return undefined;

Let's rename |trackingId| to |primaryPointerId|.  That makes more
sense in the way it's used here.

Use this style

  if (!foo) {
    return bar;
  }

>   onTouchStart: function cp_onTouchStart(evt) {
>+    let target, screenX, screenY;
>+    if (this.evtFilter == 'touch') {
>+      if ('trackingId' in this) {

Add a comment that this is ignoring touchstarts for pointers other
than the primary one.

>+  isScrolling: false, // Scrolling gesture is executed in BrowserElementScrolling
>   onTouchMove: function cp_onTouchMove(evt) {

>+      if (evt.touches.length > 1 || !firstTouch)
>+        return;

Add a comment here that this is ignoring possible multi-touch gestures
that APZC will be handling.

>+    let success = this.scrollCallback(delta.scale(-1));

Use a more descriptive name than |success|.

Did you test all the cases that regressed from bug 814252?

Would like to see the next version of this patch, and make sure that
bug 814252 regressions have been re-tested.
Attachment #695595 - Flags: review?(jones.chris.g)
Duplicate of this bug: 825688
Created attachment 696980 [details] [diff] [review]
use touch event for scrolling when APZC is enabled, v2

1. update patch according to comment 15.
2. change the usage of isScrolling to detectingScrolling, like what we do in the patch for bug 823619.
3. all the regressions in bug 814252 were re-tested and passed.

We can use this approach if we found patch in bug 823619 is to risky for V1.
Attachment #695595 - Attachment is obsolete: true
Attachment #696980 - Flags: review?(jones.chris.g)
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Fixed by bug 823619.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 696980 [details] [diff] [review]
use touch event for scrolling when APZC is enabled, v2

Thanks for all the work on this, Shih-Chiang! :)
Attachment #696980 - Flags: review?(jones.chris.g)
status-b2g18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Duplicate of this bug: 780174
Duplicate of this bug: 828521

Comment 22

6 years ago
I filed bug 828521 which was duped to this one. I just updated my test device - build identifier: 20130108070203 and I can now scroll the support website but only every other scroll gesture works. Here is a video that demonstrates the behavior: http://people.mozilla.org/~mverdi/video/sumo-scroll.webm
(In reply to Verdi [:verdi] from comment #22)
> I filed bug 828521 which was duped to this one. I just updated my test
> device - build identifier: 20130108070203 and I can now scroll the support
> website but only every other scroll gesture works. Here is a video that
> demonstrates the behavior:
> http://people.mozilla.org/~mverdi/video/sumo-scroll.webm
bug 828367 has been filed for the problem you encountered. :)
UCID browser-012
Testcase found here https://moztrap.mozilla.org/results/case/61109/
Whiteboard: testrun 2

Comment 25

6 years ago
Issue repros on unagi build 20130115070201 Kernel from Dec 5th Nothing is able to move in the iframe.

Comment 27

6 years ago
I repro the steps that were listed below....

1. go to http://www.w3schools.com/tags/att_iframe_scrolling.asp
2. select "try it yourself"
3. go to the first iframe where the scrollbars are shown
4. try to pan in the iframe

Expected: iframe moves
Actual : nothing moves in the iframe.

Comment 28

6 years ago
I do apologizes I was able to power cycle my device and test again and it did work.  Issue is fixed and verified on build 20130115070201
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.