Closed Bug 817845 Opened 12 years ago Closed 12 years ago

[Browser] can't scroll in scrollable iframe

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

VERIFIED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: nhirata, Assigned: schien)

References

Details

(Whiteboard: testrun 2)

Attachments

(1 file, 1 obsolete file)

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
Closed: 12 years ago
Resolution: --- → DUPLICATE
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.
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)
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
Closed: 12 years ago12 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)
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
Issue repros on unagi build 20130115070201 Kernel from Dec 5th Nothing is able to move in the iframe.
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.
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.

Attachment

General

Created:
Updated:
Size: