Status

defect
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: stechz, Assigned: wesj)

Tracking

Trunk
Firefox 4.0
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

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

Updated

9 years ago
Blocks: 586288
Assignee

Updated

9 years ago
Assignee: nobody → wjohnston
Assignee

Comment 1

9 years ago
Posted patch WIP (obsolete) — Splinter Review
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.
Attachment #486369 - Flags: feedback?(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?
Attachment #486369 - Flags: feedback?(mbrubeck) → feedback+
Assignee

Comment 3

9 years ago
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.
Attachment #486369 - Flags: feedback+ → feedback?(ben)
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Assignee

Comment 4

9 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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...
Attachment #486369 - Attachment is obsolete: true
Attachment #495960 - Flags: review?(mbrubeck)
Attachment #486369 - Flags: feedback?(ben)
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.
Attachment #495960 - Flags: review?(mbrubeck)
Attachment #495960 - Flags: review?(ben)
Attachment #495960 - Flags: review+
Assignee

Comment 6

9 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #495960 - Attachment is obsolete: true
Attachment #496011 - Flags: review?(ben)
Attachment #495960 - Flags: review?(ben)
Reporter

Comment 7

9 years ago
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.
Attachment #496011 - Flags: review?(ben)
Assignee

Comment 8

9 years ago
Posted patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #496011 - Attachment is obsolete: true
Attachment #496933 - Flags: review?(ben)
Reporter

Comment 9

9 years ago
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.
Attachment #496933 - Flags: review?(ben) → review+
Assignee

Comment 10

9 years ago
Talked to mfinkle about this. Want to match better what is going on in 605618,
or wait until 605618 lands to refactor this patch
Whiteboard: [fennec-checkin-postb3]
Whiteboard: [fennec-checkin-postb3]
Assignee

Comment 11

9 years ago
Posted patch WIP Refactored (obsolete) — Splinter Review
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.
Attachment #496933 - Attachment is obsolete: true
Assignee

Comment 12

9 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Using new APIs and whatnot.
Attachment #502871 - Attachment is obsolete: true
Attachment #505291 - Flags: review?(ben)
Comment on attachment 505291 [details] [diff] [review]
patch v3

># HG changeset patch
># User Wes Johnston <wjohnston@mozilla.com>
># 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
Assignee

Comment 14

9 years ago
Posted patch Patch v4 (obsolete) — Splinter Review
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).
Attachment #505291 - Attachment is obsolete: true
Attachment #505433 - Flags: review?(ben)
Attachment #505291 - Flags: review?(ben)
>+            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
Assignee

Comment 16

9 years ago
Posted patch Patch v4Splinter Review
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.
Attachment #505433 - Attachment is obsolete: true
Attachment #505467 - Flags: review?(ben)
Attachment #505433 - Flags: review?(ben)
Ben - review ping
Reporter

Comment 18

9 years ago
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.
Attachment #505467 - Flags: review?(ben) → review+
Assignee

Comment 19

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

Pushed:
http://hg.mozilla.org/mobile-browser/rev/47fcd80acad0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Not sure how to verify this, this makes xul:scrollboxes scrollable or something?
Assignee

Comment 21

8 years ago
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.
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
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 4.0
You need to log in before you can comment on or make changes to this bug.