Last Comment Bug 458741 - need to be able to scroll subframes (e.g. iframe)
: need to be able to scroll subframes (e.g. iframe)
Status: VERIFIED FIXED
: uiwanted
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Future
Assigned To: Ben Combee (:bcombee)
:
Mentors:
: 460472 460514 461840 464769 472370 476577 476970 (view as bug list)
Depends on: 487693
Blocks: 477628 460472 460678 467670 492831 497347 512428
  Show dependency treegraph
 
Reported: 2008-10-06 09:31 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2009-09-22 10:06 PDT (History)
18 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v0.2 (2.96 KB, patch)
2008-12-22 13:23 PST, Stuart Parmenter
no flags Details | Diff | Splinter Review
Patch to InputHandler.js to enable dragging of chrome lists (12.48 KB, patch)
2009-02-20 14:24 PST, Ben Combee (:bcombee)
mark.finkle: review-
Details | Diff | Splinter Review
rev 2 of Ben's drag patch (15.29 KB, patch)
2009-02-23 09:04 PST, Ben Combee (:bcombee)
mark.finkle: review+
Details | Diff | Splinter Review
rev 3 of Ben's patch (15.41 KB, patch)
2009-02-23 13:39 PST, Ben Combee (:bcombee)
mark.finkle: review+
pavlov: review+
Details | Diff | Splinter Review
More input handler changes to deal with scrolling and clicking on chrome items (19.69 KB, patch)
2009-02-27 11:48 PST, Ben Combee (:bcombee)
mark.finkle: review+
Details | Diff | Splinter Review
fix panning (405 bytes, patch)
2009-03-01 14:29 PST, (dormant account)
ben.combee: review+
Details | Diff | Splinter Review
NOT-WORKING patch for iframe scrolling (3.93 KB, patch)
2009-05-11 09:08 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
This works with some cases! (3.77 KB, patch)
2009-05-12 16:00 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
Updated iframe scrolling patch, applied to current tree (6.23 KB, patch)
2009-06-10 13:42 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
Much simpler version, works well with single iframe, not tested with more yet (5.73 KB, patch)
2009-06-10 14:59 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
Updated to work well with top of tree, at least with invalidation patch installed (9.66 KB, patch)
2009-06-24 00:40 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
Update #6 - this one will push inner frames against outer ones, allowing scrolling away from all-screen frames (10.89 KB, patch)
2009-06-25 18:45 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
Update #7 - some code cleanup from last version (8.98 KB, patch)
2009-07-01 14:52 PDT, Ben Combee (:bcombee)
no flags Details | Diff | Splinter Review
Update #8 - added FRAMESET handling too (9.44 KB, patch)
2009-07-15 14:01 PDT, Ben Combee (:bcombee)
mark.finkle: review+
Details | Diff | Splinter Review
Try #9 - updated with Mark and Gavin's suggestions (9.92 KB, patch)
2009-07-21 09:07 PDT, Ben Combee (:bcombee)
mark.finkle: review+
Details | Diff | Splinter Review
Try #10 with screenToClient and kinetic fixes (16.52 KB, patch)
2009-07-29 08:20 PDT, Mark Finkle (:mfinkle) (use needinfo?)
ben.combee: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2008-10-06 09:31:41 PDT

    
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-10-19 01:16:37 PDT
*** Bug 460514 has been marked as a duplicate of this bug. ***
Comment 2 Stuart Parmenter 2008-12-22 13:23:22 PST
Created attachment 354216 [details] [diff] [review]
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 2008-12-23 10:53:33 PST
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?) 2008-12-23 11:54:00 PST
I'm pretty certain this is blocking 1.0
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2009-01-06 08:56:37 PST
*** Bug 461840 has been marked as a duplicate of this bug. ***
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2009-01-06 17:26:47 PST
*** Bug 472370 has been marked as a duplicate of this bug. ***
Comment 7 timeless 2009-01-06 17:53:53 PST
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 8 Mark Finkle (:mfinkle) (use needinfo?) 2009-02-02 20:31:49 PST
*** Bug 476577 has been marked as a duplicate of this bug. ***
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2009-02-04 17:29:12 PST
*** Bug 476970 has been marked as a duplicate of this bug. ***
Comment 10 Ben Combee (:bcombee) 2009-02-05 07:48:59 PST
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) 2009-02-05 07:52:04 PST
Test cases from duplicated bugs: large scrollable lists like bookmarks, history, awesomebar.
Comment 12 Ben Combee (:bcombee) 2009-02-17 10:49:30 PST
*** Bug 460472 has been marked as a duplicate of this bug. ***
Comment 13 Ben Combee (:bcombee) 2009-02-20 14:24:25 PST
Created attachment 363397 [details] [diff] [review]
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?) 2009-02-22 21:20:32 PST
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) 2009-02-23 08:22:57 PST
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) 2009-02-23 09:04:52 PST
Created attachment 363689 [details] [diff] [review]
rev 2 of Ben's drag patch

Updated with changes suggested by Mark Finkle
Comment 17 Ben Combee (:bcombee) 2009-02-23 09:06:39 PST
*** Bug 464769 has been marked as a duplicate of this bug. ***
Comment 18 Ben Combee (:bcombee) 2009-02-23 09:33:05 PST
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?) 2009-02-23 11:54:27 PST
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) 2009-02-23 12:19:09 PST
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) 2009-02-23 13:04:09 PST
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) 2009-02-23 13:39:16 PST
Created attachment 363755 [details] [diff] [review]
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?) 2009-02-24 22:48:49 PST
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) 2009-02-27 11:48:36 PST
Created attachment 364557 [details] [diff] [review]
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?) 2009-02-27 12:48:00 PST
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) 2009-02-27 13:48:24 PST
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?) 2009-02-27 14:18:39 PST
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) 2009-03-01 14:29:04 PST
Created attachment 364812 [details] [diff] [review]
fix panning

Browser.canvasBrowser.prepareForPanning() seems to have disappeared when patch was updated.
Comment 29 Ben Combee (:bcombee) 2009-03-02 09:29:34 PST
Comment on attachment 364812 [details] [diff] [review]
fix panning

I removed an extra call
Comment 30 (dormant account) 2009-03-03 16:13:24 PST
pushed my fix http://hg.mozilla.org/mobile-browser/rev/69e5be5aa154
Comment 31 Ben Combee (:bcombee) 2009-03-10 09:37:16 PDT
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?) 2009-03-13 23:58:22 PDT
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) 2009-04-12 05:19:11 PDT
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) 2009-05-11 09:08:23 PDT
Created attachment 376726 [details] [diff] [review]
NOT-WORKING patch for iframe scrolling
Comment 35 Ben Combee (:bcombee) 2009-05-12 16:00:28 PDT
Created attachment 377029 [details] [diff] [review]
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) 2009-06-10 13:42:16 PDT
Created attachment 382574 [details] [diff] [review]
Updated iframe scrolling patch, applied to current tree
Comment 37 Ben Combee (:bcombee) 2009-06-10 14:39:32 PDT
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) 2009-06-10 14:59:30 PDT
Created attachment 382593 [details] [diff] [review]
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) 2009-06-22 09:18:16 PDT
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) 2009-06-23 10:16:05 PDT
Really need change in bug 487693 to get this to display right
Comment 41 Ben Combee (:bcombee) 2009-06-24 00:40:45 PDT
Created attachment 384827 [details] [diff] [review]
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) 2009-06-24 09:25:12 PDT
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) 2009-06-24 09:27:40 PDT
Per finkle, lets push to b3, but try to get it done ASAP.
Comment 44 Mark Finkle (:mfinkle) (use needinfo?) 2009-06-24 11:07:01 PDT
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) 2009-06-25 18:45:32 PDT
Created attachment 385293 [details] [diff] [review]
Update #6 - this one will push inner frames against outer ones, allowing scrolling away from all-screen frames
Comment 46 Joel Maher ( :jmaher) 2009-06-26 09:23:45 PDT
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] 2009-06-26 10:04:45 PDT
Another testcase 

http://www.haaretz.com/

Tested with 20090626 Fennec/1.0b2
Comment 48 Ben Combee (:bcombee) 2009-06-26 10:27:59 PDT
The creativecommons search engine works much better with current patch
Comment 49 Ben Combee (:bcombee) 2009-06-26 10:31:03 PDT
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) 2009-06-26 10:38:59 PDT
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) 2009-07-01 14:52:23 PDT
Created attachment 386380 [details] [diff] [review]
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) 2009-07-15 13:37:18 PDT
Here's a test page for FRAMESET scrolling: http://people.mozilla.com/~bcombee/fennec/frame-test.html
Comment 53 Ben Combee (:bcombee) 2009-07-15 13:59:27 PDT
*** Bug 501823 has been marked as a duplicate of this bug. ***
Comment 54 Ben Combee (:bcombee) 2009-07-15 14:01:25 PDT
Created attachment 388778 [details] [diff] [review]
Update #8 - added FRAMESET handling too
Comment 55 Mark Finkle (:mfinkle) (use needinfo?) 2009-07-17 12:58:46 PDT
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: gavin@gavinsharp.com] 2009-07-17 22:40:20 PDT
(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?) 2009-07-18 08:07:49 PDT
(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) 2009-07-21 09:07:15 PDT
Created attachment 389697 [details] [diff] [review]
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?) 2009-07-29 08:20:53 PDT
Created attachment 391341 [details] [diff] [review]
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?) 2009-07-29 09:07:46 PDT
pushed: https://hg.mozilla.org/mobile-browser/rev/e51d93060779
Comment 61 Aakash Desai [:aakashd] 2009-09-22 10:06:52 PDT
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

Note You need to log in before you can comment on or make changes to this bug.