605630 - Make XUL pannable

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Benjamin Stover (:stechz)]

I believe XUL content lives in a scroll frame, so scrolling the window is not what we want to do for XUL.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP

Here's a WIP to save my place, and to see if this is the way we want to go. This actually lets up use our default dragger on local pages. I had to make it so that we actually send a mousedown when you mousedown, rather than waiting for a mouseup event, but click/command events aren't sent unless the mouse-down and mouse-up events are close enough together, so right now I'm resending the mouse-down again when you mouse-up. 

I also am trying to catch when you reach an edge, and switch to the next dragger up the tree. This doesn't always work right now and so (for example) the urlbar can come down at the wrong time.

Figured I would ask you for feedback mbrubeck. Do you think this is a worthwhile way to go? Should I be more forceful about sending these mousedown events only if we're in a local tab.

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 486369 [details] [diff] [review]
WIP

(In reply to comment #1)
> This actually lets up use our default dragger on local pages. I had to make it
> so that we actually send a mousedown when you mousedown, rather than waiting
> for a mouseup event, but click/command events aren't sent unless the mouse-down
> and mouse-up events are close enough together, so right now I'm resending the
> mouse-down again when you mouse-up. 

Hmm, I have no idea what the consequences of the extra mousedown would be.  Rather than sending a second mousedown, should we suppress it if one was already sent?

Why is the mousedown needed for XUL panning?

> Figured I would ask you for feedback mbrubeck. Do you think this is a
> worthwhile way to go? Should I be more forceful about sending these mousedown
> events only if we're in a local tab.

Restricting this to local content sounds like a good idea.  If nothing else, it would mystify any web developers who were debugging event handlers in Fennec.

The general approach seems like it should work, but I'm a bit wary of the complexity (given how complex this code is already).  I'd ask stechz for feedback on this approach too, if you haven't already.  (Also, random thought: could we reimplement about:config as HTML instead of XUL?)

Some miscellaneous comments on the code.  I know it's a work in progress, but asking questions helps me read it closely and figure out which parts I understand.

>+++ b/chrome/content/content.js

>   receiveMessage: function receiveMessage(aMessage) {
>     let json = aMessage.json;
>     let x = json.x;
>     let y = json.y;
>     let modifiers = json.modifiers;
>+    let element = null;

Not sure why this is here.

>           this._sendMouseEvent("mousemove", element, x, y);
>           this._sendMouseEvent("mousedown", element, x, y);
>           this._sendMouseEvent("mouseup", element, x, y);
>+          this._sendMouseEvent("mouseout",  element, x, y);

Does this regress bug 542735 or other sites that use mouseover/mouseout for "hover" states?

>+++ b/chrome/content/input.js

>   _onMouseDown: function _onMouseDown(aEvent) {
>+    let isContent = false;
>+    if (aEvent.target.ownerDocument.defaultView != window)
>+      isContent = true;
>+
>+    // walk up the DOM tree in search of nearest scrollable ancestor.  nulls are
>+    // returned if none found.
>+    this._target = aEvent.target;
>+    let [targetScrollbox, targetScrollInterface, dragger] = ScrollUtils.getScrollboxFromElement(this._target);
>+
>     let dragData = this._dragData;
>+    if (dragData.dragging && !targetScrollbox && isContent)
>+      return;

Can you add a comment to explain what it means if we hit this "return"?

>+    if (!isContent) {
>+      let event = document.createEvent("Events");
>+      event.initEvent("TapDown", true, true);
>+      event.clientX = aEvent.clientX;
>+      event.clientY = aEvent.clientY;
>+      let success = aEvent.target.dispatchEvent(event);
>+      if (success) {
>+          this._recordEvent(aEvent);
>+        this._target = aEvent.target;

Nit: The last line above is not needed anymore.

>   _dragBy: function _dragBy(dX, dY) {
>     let dragData = this._dragData;
>     let dragged = this._dragger.dragMove(dX, dY, this._targetScrollInterface);
>+
>+    let target = this.target;

Should this be "this._target"?

>+    while (!dragged && target) {
>+      // look up the tree for the next dragger and try dragging it
>+      target = target.parentNode;
>+      let [newTarget, targetScrollInterface, dragger] = ScrollUtils.getScrollboxFromElement(target);
>+      if(newTarget && dragger) {
>+        target = newTarget;
>+        dragged = dragger.dragMove(dX, dY, targetScrollInterface);
>+      } else {
>+        target = null;
>+      }
>+    }

I wonder if we should fold this into getScrollBoxFromElement somehow, so that we aren't doing two nested traversals up the tree.

>+    if (!target) {
>+      target = document.getElementById("inputhandler-overlay");
>+      let [targetScrollbox, targetScrollInterface, dragger] = ScrollUtils.getScrollboxFromElement(target);
>+      if(dragger)
>+        dragged = dragger.dragMove(dX, dY, targetScrollInterface);
>+
>+    }

At a glance, this seems like a dangerous fallback - do we really want to start panning the main UI just because we dragged in some element like a textbox that was not scrollable?

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 486369 [details] [diff] [review]
WIP

The inital mousedown is required to attach a dragger to the xul scrollbox while you are dragging. There is actually a strange (desktop only) bug right now, where you can fire up about:blank, click on a row (mouse down and up) and then move the mouse around and watch the list pan. That is because we fire a mousedown on the element when you mouseup, which bubbles back up to input.js and has a dragger assigned to it. I'm not sure why the mouseup doesn't make it.

To do scrolling, we need the first mousedown to occur when you actually put your finger down.

I talked to stechz a bit about this, but I'll put in for his feedback as well. Thanks.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Had to take a different approach here. On mousedown, this attaches an object to the content window named scrollObject. For local tabs, the scroller looks for this object on the window before it scrolls, and scrolls it instead.

Then there's some stuff to adjust coordinates for click events and tap highlighting. And finally, there's a little chunk to ignore mousedown or mouseup events that propagate up to input.js.

There's still one bug left here, that existed before this patch. So its not a regression, and its not even necessarily related. Just annoying. On the first tap on the content area, taps are sent to the input overlay element. On your second tap however, events are dispatched onto the content document. I think focus is being shifted somehow when you click, so that events are sent under the input overlay... or something like that? Still looking into it...

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 495960 [details] [diff] [review]
Patch v1

>       <method name="scrollBy">
>         <parameter name="x"/>
>         <parameter name="y"/>
>         <body>
>           <![CDATA[
>             let position = this.getPosition();
>-            x = Math.floor(Math.max(0, Math.min(this.contentDocumentWidth,  position.x+x))-position.x);
>-            y = Math.floor(Math.max(0, Math.min(this.contentDocumentHeight, position.y+y))-position.y);
>-            this.contentWindow.scrollBy(x, y);
>+            let width  = this.contentDocumentWidth;
>+            let height = this.contentDocumentHeight;
>+
>+            let scrollObj = this.contentWindow.scrollObject;
>+            if (scrollObj) {
>+               let w = {},
>+                   h = {};
>+               scrollObj.getScrolledSize(w, h);
>+               width = w.value;
>+               height = h.value;
>+            }
>+
>+            x = Math.floor(Math.max(0, Math.min(width,  position.x+x))-position.x);
>+            y = Math.floor(Math.max(0, Math.min(height, position.y+y))-position.y);
>+            if (scrollObj)
>+               scrollObj.scrollBy(x,y);
>+            else
>+               this.contentWindow.scrollBy(x, y);
>           ]]>
>         </body>
>       </method>
> 
>       <!-- Scroll viewport to (x, y) offset of document in device pixels. -->
>       <method name="scrollTo">
>         <parameter name="x"/>
>         <parameter name="y"/>
>         <body>
>           <![CDATA[
>-            x = Math.floor(Math.max(0, Math.min(this.contentDocumentWidth,  x)));
>-            y = Math.floor(Math.max(0, Math.min(this.contentDocumentHeight, y)));
>-            this.contentWindow.scrollTo(x, y);
>+            let width  = this.contentDocumentWidth;
>+            let height = this.contentDocumentHeight;
>+
>+            let scrollObj = this.contentWindow.scrollObject;
>+            if (scrollObj) {
>+               let w = {},
>+                   h = {};
>+               scrollObj.getScrolledSize(w, h);
>+               width = w.value;
>+               height = h.value;
>+            }
>+
>+            x = Math.floor(Math.max(0, Math.min(width,  x)));
>+            y = Math.floor(Math.max(0, Math.min(height, y)));
>+            if (scrollObj)
>+               scrollObj.scrollTo(x,y);
>+            else
>+               this.contentWindow.scrollTo(x, y);
>           ]]>
>         </body>
>       </method>

Please factor out the common code here - possibly by having scrollBy call scrollTo or vice-versa (like in the <browser remote> binding).

This code looks good to me, but stechz might know more about this, and also how it will interact with bug 605618.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Moved those bits into a function, _getScrollSize. I'd like to know how this is going to fit with the iframe scrolling bits too, so asking stechz for review.

I figured out that if I use setting elements for all of the items in our richlistbox, rather than just for the settings being edited, I remove the problems I saw with the document stealing events. So I am suspicious that the hiding/showing of the editor row is what is causing problems. I'd like to move to just using setting elements, maybe with some CSS to show a "Reset" button. Not sure if there are strong opinions about that or not. If I don't hear anything here, I'll file a separate bug.

Comment 7

[Benjamin Stover (:stechz)]

Comment on attachment 496011 [details] [diff] [review]
Patch v2

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>+      <!-- Get current scrollbox size -->
>+      <method name="_getScrollSize">
>+         <body>
>+           <![CDATA[
>+            let scrollObj = this.contentWindow.scrollObject;
>+            if (scrollObj) {
>+               let w = {},
>+                   h = {};
>+               scrollObj.getScrolledSize(w, h);
>+               return [w.value, h.value];
>+            }
>+            return [this.contentDocumentWidth, this.contentDocumentHeight];
>+           ]]>
>+         </body>
>+      </method>
>+

This will conflict with iframe panning, but it can be my problem since this will more than likely land first.

>-            x = Math.floor(Math.max(0, Math.min(this.contentDocumentWidth,  position.x+x))-position.x);
>-            y = Math.floor(Math.max(0, Math.min(this.contentDocumentHeight, position.y+y))-position.y);
>-            this.contentWindow.scrollBy(x, y);
>+            let [width, height] = this._getScrollSize();
>+
>+            x = Math.floor(Math.max(0, Math.min(width,  position.x+x))-position.x);
>+            y = Math.floor(Math.max(0, Math.min(height, position.y+y))-position.y);
>+
>+            if (this.contentWindow.scrollObject)
>+               this.contentWindow.scrollObject.scrollBy(x,y);
>+            else
>+               this.contentWindow.scrollBy(x, y);

I know this isn't your doing, but please space out the operators +/-/etc.

>-            x = Math.floor(Math.max(0, Math.min(this.contentDocumentWidth,  x)));
>-            y = Math.floor(Math.max(0, Math.min(this.contentDocumentHeight, y)));
>-            this.contentWindow.scrollTo(x, y);
>+            let [width, height] = this._getScrollSize();
>+
>+            x = Math.floor(Math.max(0, Math.min(width,  x)));
>+            y = Math.floor(Math.max(0, Math.min(height, y)));
>+
>+            if (this.contentWindow.scrollObject)
>+               this.contentWindow.scrollObject.scrollTo(x,y);
>+            else
>+               this.contentWindow.scrollTo(x, y);

I agree with what mbrubeck said about refactoring here.

>diff --git a/chrome/content/content.js b/chrome/content/content.js
>--- a/chrome/content/content.js
>+++ b/chrome/content/content.js
>@@ -137,16 +137,25 @@ const ElementTouchHelper = {
>  * @param x,y Browser coordinates
>  * @return Element at position, null if no active browser or no element found
>  */
> function elementFromPoint(x, y) {
>   // browser's elementFromPoint expect browser-relative client coordinates.
>   // subtract browser's scroll values to adjust
>   let cwu = Util.getWindowUtils(content);
>   let scroll = Util.getScrollOffset(content);
>+  
>+  if (content.scrollObject) {
>+    let x = {},
>+        y = {};
>+    content.scrollObject.getPosition(x,y);
>+    scroll.x += x.value;
>+    scroll.y += y.value;
>+  }
>+

I'm concerned that you are fixing a specific instance of a more general problem here. It should not matter that you are panning a XUL scrollbox; that shouldn't specifically factor in. What if it's a scrollbox you aren't panning that isn't at a scroll offset of (0, 0)?

> 
>+  if (Util.isParentProcess()) {
>+    let scroll = aElement;
>+    while (scroll) {
>+      if (scroll && scroll.scrollBoxObject) {
>+        let x = {}, y = {};
>+        scroll.scrollBoxObject.getPosition(x,y);
>+        offset.add(x.value, y.value);
>+      }
>+      scroll = scroll.parentNode;
>+    }
>+  }
>+

Same thing here.

>+
>+        if (Util.isParentProcess()) {
>+          let scrollbox = element;
>+          while (scrollbox && !scrollbox.scrollBoxObject) {
>+            scrollbox = scrollbox.parentNode;
>+          }
>+          if (scrollbox)
>+            content.scrollObject = scrollbox.scrollBoxObject;
>+        }
>         break;

We need comments here about what we're doing. Why are we specifically looking for scrollBoxObjects? Why are we setting this property? Where is it used? Similar comments would be useful in browser.xml.

>+    if (content.scrollObject) {
>+      let scrollX = { value: 0 },
>+          scrollY = { value: 0 };
>+      content.scrollObject.getPosition(scrollX, scrollY);
>+      aX -= scrollX.value;
>+      aY -= scrollY.value;
>+    }
>+

What is this for?

>   _onMouseDown: function _onMouseDown(aEvent) {
>+    if (aEvent.originalTarget.ownerDocument && aEvent.originalTarget.ownerDocument.defaultView != window)
>+      return;
>+

This feels like it is Fennec specific, so it should probably live in the main dragger for Fennec if it can.

>+    if (aEvent.originalTarget.ownerDocument && aEvent.originalTarget.ownerDocument.defaultView != window)
>+      return;
>+

Same.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Simpler and slightly leaner. Found a bug in the previous patch where returning the scrollbox position from browser.getPosition() was causing the unit problems I was having. Here I do all of the scroller work in the contentScrollboxScroller, and don't have to do any extra unit conversions.

I also removed the chunks checking for the ownerDocument. That isn't fixed (i.e. those events still propagate up to the top). MainDragger doesn't currently have any idea who threw the original event so it isn't as easy as moving this code there. We can figure out some way everyone likes to fix it in bug 607447.

Comment 9

[Benjamin Stover (:stechz)]

Comment on attachment 496933 [details] [diff] [review]
Patch v3

>       scrollBy: function(aDx, aDy) {
>-        getBrowser().scrollBy(aDx, aDy);
>+        let browser = getBrowser();
>+        // if the window contains scrollObject the user tapped in an
>+        // internal scrollbox and we should scroll
>+        if (browser.getAttribute("remote") != "true") {
>+          if (browser.contentWindow.scrollObject)
>+            browser = browser.contentWindow.scrollObject;
>+        }
>+        browser.scrollBy(aDx, aDy);

Call this something besides browser. |scroller| might do nicely.

I'm OK with checking for the remote attribute, though it seems unnecessary? It's nice because the if check tells us this is only for local tabs, so if you remove it be sure to mention "local" somewhere in the comment.

Applies to scrollTo and getPosition as well.

r+ with nits.

Comment 10

[Wesley Johnston (:wesj)]

Talked to mfinkle about this. Want to match better what is going on in 605618,
or wait until 605618 lands to refactor this patch

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP Refactored

This is mostly refactored, but still differs a bit from what is in the iframe bug. Namely, this adds getContentViewAt, getRootContentView, and isRoot methods directly to browser bindings, along with a new getParentContentView to be used for nested scrollboxes (not implemented correctly yet). I also modified the browser getPosition function to more closely match what scrollboxes do.

Then this modifies the rest of the panning and scrolling code to use this. Most of that is taken directly from the patches in bug 605618 (with a little modification for the new getPosition function).

Bug 605618 is awfully close to landing though, and I don't want to hold it up by forcing the mb patches to be refactored (more), so I'm mostly just putting this up for reference/driveby feedback for now.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: patch v3

Using new APIs and whatnot.

Comment 13

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

Comment on attachment 505291 [details] [diff] [review]
patch v3

># HG changeset patch
># User Wes Johnston <[e-mail]>
># Parent 748bbf883616aea80c4e29a8494326ba175e7e88
>Bug 605630 - Scroll scrollboxes in local tabs
>
>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>--- a/chrome/content/bindings/browser.xml
>+++ b/chrome/content/bindings/browser.xml
>@@ -428,20 +428,66 @@
>       <method name="getRootView">
>         <body>
>           <![CDATA[
>             return this._contentView;
>           ]]>
>         </body>
>       </method>
> 
>+      <field name="_contentViewPrototype"><![CDATA[
>+        ({
>+          self: this,
>+          _elt: null,

_elt -> _element

>+          init: function(aElement) {
>+            let self = this.self;

Not needed?

>+          isRoot: function() {
>+            this._elt == this;

return this._elt == this.self;   ???

>       <method name="getViewAt">

>+            let cwu = this.contentDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

sadly, Ci is not a sure thing here so use Components.interfaces and wrap if needed

>+            while (elt && !elt.scrollBoxObject) {
>+              elt = elt.parentNode;
>+            }

No {} needed

>+            if (!elt)
>+              return this._contentView;
>+
>+            var cv = Object.create(this._contentViewPrototype);

var -> let

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Whoops. Thanks for the catch. Removed all references to self and the element (although it might be nice to have later if we ever run into a nested scrollbox situation).

Comment 15

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

>+            let cwu = this.contentDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

sadly, Ci is not a sure thing here so use Components.interfaces and wrap if
needed

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Dang it. Copy and paste strikes again. Did find some other places in this same file where we are using Cc and Ci though, and a few references to Services.io

It also rears its head in dialog.xml, and Services.prefs appears a whole lot in setting.xml. Filing a separate bug for those.

Comment 17

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

Ben - review ping

Comment 18

[Benjamin Stover (:stechz)]

Comment on attachment 505467 [details] [diff] [review]
Patch v4

I love this patch!

I don't understand why extra Services.io => this._ios stuff is in here, but it looks right.

Comment 19

[Wesley Johnston (:wesj)]

Just trying to remove all references to Services, CC, Ci, etc in there.

Pushed:
http://hg.mozilla.org/mobile-browser/rev/47fcd80acad0

Comment 20

[Martijn Wargers (dead)]

Not sure how to verify this, this makes xul:scrollboxes scrollable or something?

Comment 21

[Wesley Johnston (:wesj)]

Main goal here was to make about:config panable, so that is what I would check. But yes, it makes XUL:Scrollboxes loaded in local pages pannable.

Comment 22

[Martijn Wargers (dead)]

Ok, thanks for the info. Marking verified fixed, because scrolling in about:config works fine. (however, the scrolling indicators don't work there, filed bug 641692 for it)

Verified fixed, using:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre

and:
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre