458741 - need to be able to scroll subframes (e.g. iframe)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Comment 2

[Stuart Parmenter]

Attachment: v0.2

this kind of works, but is pretty slow for some reason and doesn't allow you to escape the sending mouse events to the scrollbox (so you can't pull out sidebars and such)

Comment 3

[Jonathan Greene]

Given this is being used on an "internet tablet" I would think we'd want to have this solved before releasing ... It's hard but possible to login to Gmail while impossible to login to Google Reader as the form fields are off screen on the left side.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm pretty certain this is blocking 1.0

Comment 7

[timeless]

i can't tell if this patch works well for multiply nested scrollbable regions.

there are a number of ways of dealing w/ this problem, all suck in various interesting ways.

* try to auto stretch things (i have an impl of this)
* let the user stretch a single region (google chrome does this for <textarea>s, and all browsers do it for <frame>s)
* let the user see and affect focus

most people try to magically get things right - this IME fails miserably.

google reader, google spreadsheets, google maps

Comment 10

[Ben Combee (:bcombee)]

07:45 <@mfinkle> bcombee: that bug has 2 purposes
07:46 <@mfinkle> 1 pan XUL widgets
07:46 <@mfinkle> 2 pan iframes in web content
07:46 <@mfinkle> the current patch only really looks for <scrollbox> XUL widgets
07:46 <@mfinkle> we should change to look for XUL widgets that support a scrollBoxObject
07:47 <@mfinkle> bcombee: also, the patch looks for the element at the event point (x, y)
07:47 <@mfinkle> but that could be an item inside a listbox
07:47 <@mfinkle> so we really need to walk up the parents to find a scrollable XUL widget

Comment 11

[Ben Combee (:bcombee)]

Test cases from duplicated bugs: large scrollable lists like bookmarks, history, awesomebar.

Comment 13

[Ben Combee (:bcombee)]

Attachment: Patch to InputHandler.js to enable dragging of chrome lists

This patch allows scrolling of XUL elements that have a scrollBoxObject interface.  This works for the awesome bar and bookmarks.

I'd like to simplify this more by refactoring the kinetic panning input handler to eliminate duplication with the non-kinetic panning code.  This patch doesn't apply kinetic scrolling to XUL elements, leaving that for the content area only.

This patch also has a bug on desktop systems -- it doesn't track the mouse leaving the window, so if you drag off the edge, you'll still be panning when your mouse reenters the window.  I need to investigate the right way to handle this cleanly.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 363397 [details] [diff] [review]
Patch to InputHandler.js to enable dragging of chrome lists

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

>+function getScrollableXULParent(elem) {

function getScrollboxFromElement(elem)  ?

since it returns a scrollbox, not an actual element

>+  let scrollBoxInterface = null;

nit: "scrollbox" might be a nice shorter name :)

>+      if ("scrollBoxObject" in elem && elem.scrollBoxObject) {

Try removing the | "scrollBoxObject" in elem | part to see if it still works well enough

>+        break;
>+      } else {

nit: is that the style in that file? We normally don't cuddle our "else"

>+        scrollBoxInterface = elem.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);

have we defined "Ci" shortcut in this file's scope? If so, use them.

>+        break;
>+      }
>+    }
>+    catch(e) {

nit: space between "catch" and "(e)"

>@@ -56,12 +77,13 @@ function InputHandler() {
> function InputHandler() {
>   let stack = document.getElementById("browser-container");
>   stack.addEventListener("DOMMouseScroll", this, true);

nit: add a blank line here

>@@ -70,10 +92,11 @@ function InputHandler() {
>   let allowKinetic = prefsvc.getBoolPref("browser.ui.panning.kinetic");
>   //let allowKinetic = false;

remove the commented line?

>+  if (allowKinetic) {
>     this._modules.push(new KineticPanningModule(this));
>-  else
>+  } else {
>     this._modules.push(new PanningModule(this));
>+  }

nit: we try not to use "}" "{" when the "if" or both "if" and "else" blocks are a single line.

>@@ -132,6 +155,8 @@ KineticPanningModule.prototype = {
>+    scrollableXULTarget: null,

targetScrollbox ? It's not really a XUL element. It's an nsIScrollBoxObject

>+    // start kinetic scrolling here for canvas only
>+    if (dragData.scrollableXULTarget) {
>+      dragData.scrollableXULTarget.scrollBy(dragData.sX - sX, dragData.sY - sY);
>+    } else if (dragData.panningContent) {

Move the comment into this block. Put "esle if" on it's own line - no cuddles

>@@ -234,19 +280,23 @@ KineticPanningModule.prototype = {
>+    dragData.panningContent = (aEvent.target === document.getElementById("browser-canvas"));
>+    if (!dragData.panningContent)
>+      dragData.scrollableXULTarget = getScrollableXULParent(aEvent.target);

Can we remove the need for .panningContent? It looks like you check for .scrollableXULTarget then do an "else if" .panningContent in many of the new "if" blocks. The original code normally had no checks. So I think a check for .scrollableXULTarget should be enough, followed by an "else" for the assumed fallback of panning the content.

>@@ -365,8 +417,8 @@ KineticPanningModule.prototype = {
>-    let [leftVis,] = ws.getWidgetVisibility("tabs-container", false);
>-    let [rightVis,] = ws.getWidgetVisibility("browser-controls", false);
>+    let [leftVis] = ws.getWidgetVisibility("tabs-container", false);
>+    let [rightVis] = ws.getWidgetVisibility("browser-controls", false);

Hmm, those commas were there on purpose. New JS "array comprehension" is the feature being used. Basically we are saying that we know the method returns an array with 2 elements, but we don't care about the second element.

Feel free to remove more commented debug "dump" calls. We don't need them in the checked in code.

Comment 15

[Ben Combee (:bcombee)]

Mark, thanks for the review.  I can't get rid of dragData.panningContent; if it's not there, then you'll be able to pan the content area by dragging in the non-scrollable XUL elements.  I'll add a comment to clarify.

I got rid of the trailing commas in the array assignment statements because js2 mode in Emacs flags them as problematic.  However, I'll just disable that local warning, since it doesn't apply to our JavaScript engine.

Comment 16

[Ben Combee (:bcombee)]

Attachment: rev 2 of Ben's drag patch

Updated with changes suggested by Mark Finkle

Comment 18

[Ben Combee (:bcombee)]

Comment on attachment 363689 [details] [diff] [review]
rev 2 of Ben's drag patch

Adding Stuart to review too.  Note: this doesn't fix scrolling of subitems in content, only scrolling of UI elements.  Bug should be clarified or moved around, I think.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 363689 [details] [diff] [review]
rev 2 of Ben's drag patch

>-  let prefsvc = Components.classes["@mozilla.org/preferences-service;1"].
>-    getService(Components.interfaces.nsIPrefBranch2);
>+  let prefsvc = Cc["@mozilla.org/preferences-service;1"].
>+    getService(Ci.nsIPrefBranch2);

Feel free to put this on the same line - we aren't diehard 80 char people (yet)

>   _dragMove: function(sX, sY) {
>-    ws.dragMove(sX, sY);
>+    let dragData = this._dragData;
>+    if (dragData.targetScrollbox)
>+      dragData.targetScrollbox.scrollBy(dragData.sX - sX, dragData.sY - sY);
>+    else if (dragData.panningContent)
>+      ws.dragMove(sX, sY);
>   },

So you're saying that the | else if (dragData.panningContent) | is needed and that a plain old | else | wouldn't work?

Comment 20

[Ben Combee (:bcombee)]

Yes, the "else if (dragData.panningContent)" is needed to avoid panning the content area when doing things like selecting text in the address bar.

Comment 21

[Ben Combee (:bcombee)]

I've discovered one problem with the patch -- clicking in fields like the URL bar tends to fail -- this is because of the click handler trying to also click on content.  I'm updating the patch to fix this and also to refactor the panning modules into just one class to avoid code duplication.

Comment 22

[Ben Combee (:bcombee)]

Attachment: rev 3 of Ben's patch

This consolidates the kinetic and non-kinetic drag handlers into one class to simplify the code considerably.  This also modified the ClickingHandler to only respond to clicks inside the content area to avoid problems clicking on text fields like the URL bar.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

landed XUL part of this bug:
http://hg.mozilla.org/mobile-browser/rev/796019256798

Leaving open for the web content part

Comment 24

[Ben Combee (:bcombee)]

Attachment: More input handler changes to deal with scrolling and clicking on chrome items

This is the change to dispatch clicks to scrollable XUL content

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 364557 [details] [diff] [review]
More input handler changes to deal with scrolling and clicking on chrome items

At some point we should look into a way to share any duplicate code in the ChromeInputModule and the ContentPanning Module.

Comment 26

[Ben Combee (:bcombee)]

Mark, I was thinking of trying to move the kinetic code into its own module so it could be used by both.  I may move the content clicking code into the content panning module too, since I want to grab on mousedown so if you pan outside the content area, you're still moving that until you release.  I don't want to miss the mouseup.

It may make sense to refactor this into a general click/drag recognizer that would then send those events on to either a chrome or content handler.  The key idea is that a drag started on any element should remain attached to that until you lift, even if you drag outside that component's border.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/6652183761f4

This is a followup for the XUL part. Still leaving open for the web content part.

Comment 28

[(dormant account)]

Attachment: fix panning

Browser.canvasBrowser.prepareForPanning() seems to have disappeared when patch was updated.

Comment 29

[Ben Combee (:bcombee)]

Comment on attachment 364812 [details] [diff] [review]
fix panning

I removed an extra call

Comment 30

[(dormant account)]

pushed my fix http://hg.mozilla.org/mobile-browser/rev/69e5be5aa154

Comment 31

[Ben Combee (:bcombee)]

Capturing a conversation about the design of the subframe part of this.

08:45 < bcombee> stuart: so, my current thinking for content panning is that the
                 hard work would actually happen in the WidgetStack
08:45 < bcombee> we modify ws.DragStart to add the initial coordinates of the drag,
                 then the stack looks in the content to figure out just what area
                 is being dragged
08:46 < bcombee> keeps subframe complexity out of InputHandler
08:47 < bcombee> ws could do drag cascading where if you drag a subframe that's
                 already at limit, it moves the whole
08:48 < gavin> hmm, having the stack interact with content doesn't feel right
08:48 < bcombee> gavin: something has to interact with content
08:48 < bcombee> I can push that down a level
08:48 < gavin> sure
08:49 < bcombee> I want to figure out how scrollwheel is handled, as that seems to
                 have the behavior I want now
08:49 < gavin> but it seems like it'd be valuable to keep that logic out of the
               widgetstack, since all the widgetstack currently cares about are its
               widgets
08:49 < bcombee> yeah, I'll go down a level
08:50 < bcombee> I just saw the ws.startDrag calls in InputHandler but hadn't dug
                 deeper
08:50 < bcombee> those just give a delta, so that's the first change point
08:51 < bcombee> hmmm, actually, scroll wheel doesn't do the cascade scrolling
08:51 < bcombee> at least on FF 3.0 -- I just tested with a form with embedded text
                 area
08:51 < bcombee> I had to mouse out of text area for wheel to scroll page
08:53 < bcombee> ah, ws.dragStart already has coords -- misinterpreted the code
08:58 < bcombee> yeah, CanvasBrowser is the key change point -- it needs a more
                 sophisticated interface to specify start of drag
08:58 < bcombee> however, widgetstack could be affected -- if you're scrolling a
                 subframe, that means you wouldn't get the side/top panels to show
08:59 < gavin> bcombee: from widgetstack's perspective, scrolling subframes doesn't
               have any effect, does it?
08:59 < gavin> since the "page position" isn't changing
09:00 < gavin> just need to have the canvasbrowser redraw the relevant area
09:00 < bcombee> gavin: it may -- if you scrolled a subframe to its edge, the next
                 move in that area may move the whole page
09:00 < gavin> sure, in which case you just do a normal pan
09:00 < bcombee> I'm not sure how I'd handle if you started to move back
09:00 < bcombee> since you could then just move content
09:00 < gavin> but before that happens, the widgetstack doesn't need to know
09:01 < bcombee> right, but the actual movement gets funneled through ws from
                 InputHandler
09:01 < gavin> what actual movement?/
09:01 < bcombee> the input events
09:02 < gavin> my assertion is that the subframe movement doesn't need to get
               funnelled through ws
09:02 < bcombee> but InputHandler only knows how to talk to widgetstack
09:02 < gavin> we can teach it to talk to the canvasbrowser! :)
09:02 < gavin> though that could get slightly complicated, I suppose
09:03 < bcombee> yeah, although we could have inputhandler talk to canvasbrowser,
                 with browser pushing back to ws
09:03 < bcombee> ouch

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Why couldn't the ContentPanningModule simply talk to the content window of the frame and scroll it (as a first cut)?

If we wanted CanvasBrowser to work with frames, we'd need create a "CanvasFrame" for each content window (main window and frame windows). CanvasBrowser would become a manager for the CanvasFrames.

Comment 33

[Oleg Romashin (:romaxa)]

Can we just simply provide some API to domWindowUtils which will allow use to get scrollableView for element/point, and scroll that view as we want?

Comment 34

[Ben Combee (:bcombee)]

Attachment: NOT-WORKING patch for iframe scrolling

Comment 35

[Ben Combee (:bcombee)]

Attachment: This works with some cases!

This is a better patch, but still not 100%.  It works on the test page I've setup at http://combee.net/fennec/iframe-test.html, but I suspect it would fail if the iframe was nested in other elements.

Comment 36

[Ben Combee (:bcombee)]

Attachment: Updated iframe scrolling patch, applied to current tree

Comment 37

[Ben Combee (:bcombee)]

Comment on attachment 382574 [details] [diff] [review]
Updated iframe scrolling patch, applied to current tree

Removing review -- this will be revised shortly

Comment 38

[Ben Combee (:bcombee)]

Attachment: Much simpler version, works well with single iframe, not tested with more yet

This one removes the frameForPoint method and fixes elementFromPoint to return innermost element.  However, GMail fails because we don't yet have the code to drag the parent if the dragBy doesn't change position on the frame, so this won't be final patch.

Comment 39

[Ben Combee (:bcombee)]

Per meeting, we'll try to push this version of patch with updates for current front end, deal with nested iframes and possible zooming issues in a separate bug.  I'm getting the patch updated for top-of-tree now.

Comment 40

[Ben Combee (:bcombee)]

Really need change in bug 487693 to get this to display right

Comment 41

[Ben Combee (:bcombee)]

Attachment: Updated to work well with top of tree, at least with invalidation patch installed

This seems to have invalidation problems even with Vlad's patch when you're zoomed.  Mainly tested with http://combee.net/fennec/iframe-test.html

Comment 42

[Ben Combee (:bcombee)]

Current patch is risky -- there's no way to recover from getting an iframe taking up full screen, which means you can't pan to either sidebar.  Also, need to investigate FRAMESETs too, like the diggbar.

Comment 43

[Ben Combee (:bcombee)]

Per finkle, lets push to b3, but try to get it done ASAP.

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 384827 [details] [diff] [review]
Updated to work well with top of tree, at least with invalidation patch installed

Not ready to land

Comment 45

[Ben Combee (:bcombee)]

Attachment: Update #6 - this one will push inner frames against outer ones, allowing scrolling away from all-screen frames

Comment 46

[Joel Maher ( :jmaher ) (UTC -8)]

in the fennec testday kbrosnan pointed out that doing a search using creative commons (one of our oob search providers) returns results in an iframe that is not scrollable:
http://search.creativecommons.org/?q=test&sourceid=Mozilla-search

just another testcase

Comment 47

[Aaron Train [:aaronmt]]

Another testcase 

http://www.haaretz.com/

Tested with 20090626 Fennec/1.0b2

Comment 48

[Ben Combee (:bcombee)]

The creativecommons search engine works much better with current patch

Comment 49

[Ben Combee (:bcombee)]

http://www.haaretz.com/ isn't a good testcase for this patch -- it's using an IFRAME, but the actual page content is in a DIV which appear be scrolling like Google Maps.  Verified with DOM Inspector.

Comment 50

[Ben Combee (:bcombee)]

http://digg.com/d1utFY (a Diggbar'd page) seems to work with this patch -- I thought they were using framesets, but I was wrong.

Comment 51

[Ben Combee (:bcombee)]

Attachment: Update #7 - some code cleanup from last version

This should be ready.  I developed it on top of kinetic stop patch, so that may need to be applied for this to apply.

Comment 52

[Ben Combee (:bcombee)]

Here's a test page for FRAMESET scrolling: http://people.mozilla.com/~bcombee/fennec/frame-test.html

Comment 54

[Ben Combee (:bcombee)]

Attachment: Update #8 - added FRAMESET handling too

Comment 55

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 388778 [details] [diff] [review]
Update #8 - added FRAMESET handling too


>+    // step through layers of IFRAMEs and FRAMES to find innermost element
>+    while (elem && (elem.tagName == "IFRAME" || elem.tagName == "FRAME")) {

is elem.localName == "iframe" more appropriate here?

>+      let frameWin = elem.ownerDocument.defaultView;
>+      //dump("*** elementFromPoint: offset - " + elem.offsetLeft + "," +
>+      //    elem.offsetTop + "; winScroll - " + frameWin.scrollX +
>+      //     "," + frameWin.scrollY + "\n");
>+      x = x - elem.offsetLeft + frameWin.scrollX;
>+      y = y - elem.offsetTop + frameWin.scrollY;

not sure if the nsIDOMWindowUtils.getScrollXY method is better here or not.

>+function dumpLn() {
>+  // like dump, but each arg is handled and there's an automatic newline
>+  for (var i = 0; i < arguments.length; i++) { dump(arguments[i]); }
>+  dump("\n");
>+}
>+

WidgetStack.js has something too. Be nice if we only had one copy.

> 
>+  _dragInner: function _dragInner(dx, dy) {

Some comments here would help us remember why we need to do this panning different than other panning.

>   _dragBy: function _dragBy(dx, dy) {
>-    let panned = ws.dragBy(dx, dy);
>+    let panned = this._dragInner(dx, dy);
>+    if (!panned)
>+      panned = ws.dragBy(dx, dy);
>     return panned;
>   },

Same here

>   _dragMove: function _dragMove(sX, sY) {
>     let dragData = this._dragData;
>     [sX, sY] = dragData.lockMouseMove(sX, sY);
>-    let panned = ws.dragMove(sX, sY);
>+    let dx = sX - dragData.sX;
>+    let dy = sY - dragData.sY;
>+
>+    let panned = this._dragInner(dx, dy);
>+    if (!panned)
>+      panned = ws.dragMove(sX, sY);
>+

And here. Maybe a different name for _dragInner would  be helpful too.

_panFrame ? _dragFrame ?

>+  // this can be called to allow a screen refresh once while dragging
>+  dragUpdate: function dragUpdate() {
>+    if (!this._dragging)
>+      return;
>+
>+    this._viewportDragUpdate();
>+  },

"once while dragging" ? not clear to me

r+ with those nits handled somehow

Comment 56

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #55)
> (From update of attachment 388778 [details] [diff] [review])
> 
> >+    // step through layers of IFRAMEs and FRAMES to find innermost element
> >+    while (elem && (elem.tagName == "IFRAME" || elem.tagName == "FRAME")) {
> 
> is elem.localName == "iframe" more appropriate here?

instanceof HTMLFrameElement / HTMLIFrameElement?

Comment 57

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #56)
> (In reply to comment #55)
> > (From update of attachment 388778 [details] [diff] [review] [details])
> > 
> > >+    // step through layers of IFRAMEs and FRAMES to find innermost element
> > >+    while (elem && (elem.tagName == "IFRAME" || elem.tagName == "FRAME")) {
> > 
> > is elem.localName == "iframe" more appropriate here?
> 
> instanceof HTMLFrameElement / HTMLIFrameElement?

Oh yeah. That's the ticket!

Comment 58

[Ben Combee (:bcombee)]

Attachment: Try #9 - updated with Mark and Gavin's suggestions

I addressed and tested all the changes except for dumpLn.  I had seen the logging code in WidgetStack, and I feel we should unify that, but not for this patch.

Comment 59

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Try #10 with screenToClient and kinetic fixes

This patch adds CanvasBrowser._screenToClientCoords convertor which is needed for calling elementFromPoint. It also adds a check for dx==0 && dy==0 in _panFrame so we don't jump page jumps during the kinetic panning of frames

Comment 60

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed: https://hg.mozilla.org/mobile-browser/rev/e51d93060779

Comment 61

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2a2pre) Gecko/20090922 Fennec/1.0a3

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2a2pre) Gecko/20090922
Fennec/1.0b4pre