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)

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.

I'm pretty certain this is blocking 1.0

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

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

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

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

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.

Updated with changes suggested by Mark Finkle

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 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?

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

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.

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.

landed XUL part of this bug: http://hg.mozilla.org/mobile-browser/rev/796019256798 Leaving open for the web content part

This is the change to dispatch clicks to scrollable XUL content

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.

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.

http://hg.mozilla.org/mobile-browser/rev/6652183761f4 This is a followup for the XUL part. Still leaving open for the web content part.

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

Comment on attachment 364812 [details] [diff] [review] fix panning I removed an extra call

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

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

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.

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?

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 on attachment 382574 [details] [diff] [review] Updated iframe scrolling patch, applied to current tree Removing review -- this will be revised shortly

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.

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.

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

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

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.

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

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

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

Another testcase http://www.haaretz.com/ Tested with 20090626 Fennec/1.0b2

The creativecommons search engine works much better with current patch

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.

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

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

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

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

(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?

(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!

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.

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

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

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