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)
Tracking
()
People
(Reporter: nhirata, Assigned: schien)
References
Details
(Whiteboard: testrun 2)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Perhaps bug 817859 is related to this bug?
Comment 2•12 years ago
|
||
Never mind, bug 817859 is another cause.
Component: Gaia::Browser → DOM: Core & HTML
Product: Boot2Gecko → Core
QA Contact: nhirata.bugzilla
Updated•12 years ago
|
blocking-basecamp: ? → +
Shih-Chiang: Could this be a regression from bug 805638?
Assignee: nobody → schien
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
This sounds vaguely like an issue we had on android with the NYT site.
Comment 6•12 years ago
|
||
I think the NYT issue you're referring to is bug 750839 which was mostly NYT-specific and an evangelism issue.
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
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.
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 11•12 years ago
|
||
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 → ---
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Comment 17•12 years ago
|
||
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)
Updated•12 years ago
|
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Fixed by bug 823619.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 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)
Updated•12 years ago
|
Comment 22•12 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
Assignee | ||
Comment 23•12 years ago
|
||
(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•12 years ago
|
||
UCID browser-012
Testcase found here https://moztrap.mozilla.org/results/case/61109/
Whiteboard: testrun 2
Comment 25•12 years ago
|
||
Issue repros on unagi build 20130115070201 Kernel from Dec 5th Nothing is able to move in the iframe.
wfm. How are you testing?
Comment 27•12 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•12 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.
Description
•