Jim Mathies [:jimm] Reporter
	Description • 11 years ago

> From kats on #windev:
> For the subdocument scrolling you may want to try to use
> dom/browser-element/BrowserElementPanning.js instead of copying from the
> fennec frontend, it seems more straightforward and it's what B2G is using

	Jim Mathies [:jimm] Reporter
	Comment 2 • 11 years ago

Comment on attachment 789059 [details] [diff] [review]
subframe scroll using BrowserElementPanning

This works both with and without apz. apz is a little spotty on rendering, and could use some input tweaks. We can file follow up on those issues.

	Jim Mathies [:jimm] Reporter
	Comment 3 • 11 years ago

Comment on attachment 789059 [details] [diff] [review]
subframe scroll using BrowserElementPanning

just noticed some unused code in the panning module.

	Brian R. Bondy [:bbondy]
	Comment 5 • 11 years ago

Could you rebase this after first applying the patches for bug 898580 and bug 895905?  botond is close to landing his patch.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 11 years ago

Hmm. Bug 906877 will break this patch. Perhaps we should hold off on landing that until Metro switches over to APZC completely.

	Jim Mathies [:jimm] Reporter
	Comment 7 • 11 years ago

Fine with me, I've been working on some other bugs. Does bug 906877 somehow force us to wait until apz is turned on by default? (and what happens if someone turns it off? :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 11 years ago

I'm not sure if "depends on" is the right direction for 906877. But anyway, the WIP I have on that bug deletes some of the code in BrowserElementPanning.js that is currently not used at all, but that the patch here brings back into use. I should be able to update the WIP so that it leaves the relevant bits of BrowserElementPanning.js intact though.

	Jim Mathies [:jimm] Reporter
	Comment 9 • 11 years ago

Updated to work with the patch in bug 906877.

	Jim Mathies [:jimm] Reporter
	Comment 10 • 11 years ago

This works great without apz, but once you turn apz on it breaks - you scroll both the main page and the subframe. So there's more work to do here for apz.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 11 • 11 years ago

Can you clarify what you mean? Do you mean that with one finger drag both the main page and the subframe get scrolled? Are you sure that any existing metro code for scrolling is disabled when you turn on APZC? Only one frame should ever be scrolling at a time with APZC code.

	Jim Mathies [:jimm] Reporter
	Comment 12 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Can you clarify what you mean? Do you mean that with one finger drag both
> the main page and the subframe get scrolled? Are you sure that any existing
> metro code for scrolling is disabled when you turn on APZC? Only one frame
> should ever be scrolling at a time with APZC code.

Yes BrowserElementPanning is still active. I guess this module should be disabled when apz is enabled then.

	Jim Mathies [:jimm] Reporter
	Comment 13 • 11 years ago

This is waiting on kats landing in the parent bug.

	Jim Mathies [:jimm] Reporter
	Comment 14 • 11 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Can you clarify what you mean? Do you mean that with one finger drag both
> the main page and the subframe get scrolled? Are you sure that any existing
> metro code for scrolling is disabled when you turn on APZC? Only one frame
> should ever be scrolling at a time with APZC code.

Note that with it disabled / apz enabled, sub frame scrolling stops working. I'm guessing this is due to sub frame apz support not being in the tree yet?

	Matt Brubeck (:mbrubeck)
	Comment 15 • 11 years ago

Comment on attachment 794771 [details] [diff] [review]
subframe scroll using BrowserElementPanning

Review of attachment 794771 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/browser.js
@@ +69,5 @@
>        messageManager.loadFrameScript("chrome://browser/content/contenthandlers/ContextMenuHandler.js", true);
>        messageManager.loadFrameScript("chrome://browser/content/contenthandlers/FindHandler.js", true);
> +      messageManager.loadFrameScript("chrome://browser/content/contenthandlers/ConsoleAPIObserver.js", true);
> +      if (!Services.prefs.getBoolPref(kAsyncPanZoomEnabled)) {
> +        messageManager.loadFrameScript("chrome://global/content/BrowserElementPanning.js", true);

BrowserElementPanning appears to duplicate some functionality that is also in our current front-end code.  For example, both BrowserElementPanning.js and input.js try to set :active styles on tapped elements.  Please add something to disable that code in input.js when APZC is enabled.

I think that's the only change needed, but let's double check to make sure.

	Matt Brubeck (:mbrubeck)
	Comment 16 • 11 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> [...] For example, both BrowserElementPanning.js
> and input.js try to set :active styles on tapped elements.  Please add
> something to disable that code in input.js when APZC is enabled.

I mean, "disable that code when *BrowserElementPanning* is enabled."

	Jim Mathies [:jimm] Reporter
	Comment 17 • 11 years ago

Updated per comments. Nixed the active logic, view change events, and zoom logic.

	Jim Mathies [:jimm] Reporter
	Comment 18 • 11 years ago

Comment on attachment 795606 [details] [diff] [review]
subframe scroll using BrowserElementPanning

Review of attachment 795606 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/browser-element/BrowserElementPanning.js
@@ +60,5 @@
>                                     /* useCapture = */ false);
>        }.bind(this));
>      }
>  
> +    dump("this._isImmersive:" + this._isImmersive + "\n");

ack, removed from my local queue.

	Matt Brubeck (:mbrubeck)
	Comment 19 • 11 years ago

Comment on attachment 795606 [details] [diff] [review]
subframe scroll using BrowserElementPanning

Review of attachment 795606 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, I'm sorry; my previous comment was confusing.  I meant that we should disable the duplicate tap highlight code in the Metro "input.js" file, and eventually remove it completely when we get BrowserElementPanning enabled all the time.

	Jim Mathies [:jimm] Reporter
	Comment 20 • 11 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #19)
> Comment on attachment 795606 [details] [diff] [review]
> subframe scroll using BrowserElementPanning
> 
> Review of attachment 795606 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oops, I'm sorry; my previous comment was confusing.  I meant that we should
> disable the duplicate tap highlight code in the Metro "input.js" file, and
> eventually remove it completely when we get BrowserElementPanning enabled
> all the time.

Do you think we'll keep BrowserElementPanning? Once apz gets turned on, it gets turned off. Unless we plan to migrate what we do in input.js for non apz scrollables to BrowserElementPanning, which I don't think we'll be doing since we have all that custom dragger code there.

	Jim Mathies [:jimm] Reporter
	Comment 21 • 11 years ago

chatted with mbrubeck on irc about this. Moving input.js to BrowserElementPanning is longer term since we have a lot of metrofx specific logic there. For the work here, we'll keep what we have but rather than disable stuff in bep we'll disable stuff in input.js instead to prevent making changes to bep.

	Jim Mathies [:jimm] Reporter
	Comment 22 • 11 years ago

Ended up being a mixed bag, we still needed to turn off some stuff related to double click in bep, although we have a bug filed on getting that working for zoom so we might revisit.

	Matt Brubeck (:mbrubeck)
	Comment 23 • 11 years ago

Comment on attachment 796111 [details] [diff] [review]
subframe scroll using BrowserElementPanning v.2

Review of attachment 796111 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck but I think you can drop the BEP change.

::: dom/browser-element/BrowserElementPanning.js
@@ +62,5 @@
>      }
>  
> +    if (!this._isImmersive) {
> +      addMessageListener("Viewport:Change", this._recvViewportChange.bind(this));
> +      addMessageListener("Gesture:DoubleTap", this._recvDoubleTap.bind(this));

Why do you need to disable this?  We never send these messages in Metro Firefox, so the listeners will have no effect on us.

	Jim Mathies [:jimm] Reporter
	Comment 24 • 11 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> Comment on attachment 796111 [details] [diff] [review]
> subframe scroll using BrowserElementPanning v.2
> 
> Review of attachment 796111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=mbrubeck but I think you can drop the BEP change.
> 
> ::: dom/browser-element/BrowserElementPanning.js
> @@ +62,5 @@
> >      }
> >  
> > +    if (!this._isImmersive) {
> > +      addMessageListener("Viewport:Change", this._recvViewportChange.bind(this));
> > +      addMessageListener("Gesture:DoubleTap", this._recvDoubleTap.bind(this));
> 
> Why do you need to disable this?  We never send these messages in Metro
> Firefox, so the listeners will have no effect on us.

I guess I was just trying to be neat. I guess I could just ignore it, or ifdef it to android builds.

	Jim Mathies [:jimm] Reporter
	Comment 25 • 11 years ago

- updated per comments.

	Jim Mathies [:jimm] Reporter
	Comment 26 • 11 years ago

My mistake, no dependency anymore, since I'm not touching bep.

	Jim Mathies [:jimm] Reporter
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/b3bea36ce719

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 11 years ago

Backed out for mochitest-mc orange.
https://hg.mozilla.org/integration/fx-team/rev/dadec41b7cbc

https://tbpl.mozilla.org/php/getParsedLog.php?id=27132477&tree=Fx-Team

	Allison Naaktgeboren :ally
	Comment 29 • 11 years ago

> ::: dom/browser-element/BrowserElementPanning.js
> @@ +62,5 @@
> >      }
> >  
> > +    if (!this._isImmersive) {
> > +      addMessageListener("Viewport:Change", this._recvViewportChange.bind(this));
> > +      addMessageListener("Gesture:DoubleTap", this._recvDoubleTap.bind(this));
> 
> Why do you need to disable this?  We never send these messages in Metro
> Firefox, so the listeners will have no effect on us.

Don't we need the Gesture:DoubleTap listener for bug 895581?

	Jim Mathies [:jimm] Reporter
	Comment 30 • 11 years ago

(In reply to :Ally Naaktgeboren from comment #29)
> > ::: dom/browser-element/BrowserElementPanning.js
> > @@ +62,5 @@
> > >      }
> > >  
> > > +    if (!this._isImmersive) {
> > > +      addMessageListener("Viewport:Change", this._recvViewportChange.bind(this));
> > > +      addMessageListener("Gesture:DoubleTap", this._recvDoubleTap.bind(this));
> > 
> > Why do you need to disable this?  We never send these messages in Metro
> > Firefox, so the listeners will have no effect on us.
> 
> Don't we need the Gesture:DoubleTap listener for bug 895581?

FYI, this work isn't going to land. In a day or so we'll be turning on apz.