817845 - [Browser] can't scroll in scrollable iframe

Status: VERIFIED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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 1

[Martijn Wargers (dead)]

Perhaps bug 817859 is related to this bug?

Comment 2

[Martijn Wargers (dead)]

Never mind, bug 817859 is another cause.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Shih-Chiang: Could this be a regression from bug 805638?

Comment 4

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

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.

Comment 5

[JP Rosevear [:jpr]]

This sounds vaguely like an issue we had on android with the NYT site.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I think the NYT issue you're referring to is bug 750839 which was mostly NYT-specific and an evangelism issue.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 8

[Alex Keybl [:akeybl]]

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.

Comment 9

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

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.

Comment 11

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

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.

Comment 12

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

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.

Comment 13

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

That would require a more complicated version of attachment 694726 [details] [diff] [review] from bug 823619.  But we can keep that in mind.

Comment 14

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

Attachment: use touch event for scrolling when APZC is enabled

use touch event for scrolling if APZC is enabled.

Comment 15

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

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.

Comment 17

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

Attachment: 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.

Comment 18

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

Fixed by bug 823619.

Comment 19

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

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! :)

Comment 22

[Verdi [:verdi] Best to slack me]

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

Comment 23

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

(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. :)

Comment 24

[Cody Roesch [:croesch]]

UCID browser-012
Testcase found here https://moztrap.mozilla.org/results/case/61109/

Comment 25

[Jeni]

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

Comment 26

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

wfm.  How are you testing?

Comment 27

[Jeni]

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

[Jeni]

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