901607 - Add front end sub-frame scroll support

Status: RESOLVED WONTFIX

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Jim Mathies [:jimm]]

> 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

Comment 1

[Jim Mathies [:jimm]]

Attachment: subframe scroll using BrowserElementPanning

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[Jim Mathies [:jimm]]

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

just noticed some unused code in the panning module.

Comment 4

[Jim Mathies [:jimm]]

Attachment: subframe scroll using BrowserElementPanning

Comment 5

[Brian R. Bondy [:bbondy]]

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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

Comment 7

[Jim Mathies [:jimm]]

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[Jim Mathies [:jimm]]

Attachment: subframe scroll using BrowserElementPanning

Updated to work with the patch in bug 906877.

Comment 10

[Jim Mathies [:jimm]]

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.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 13

[Jim Mathies [:jimm]]

Attachment: subframe scroll using BrowserElementPanning

This is waiting on kats landing in the parent bug.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 15

[Matt Brubeck (:mbrubeck)]

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.

Comment 16

[Matt Brubeck (:mbrubeck)]

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

Comment 17

[Jim Mathies [:jimm]]

Attachment: subframe scroll using BrowserElementPanning

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

Comment 18

[Jim Mathies [:jimm]]

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.

Comment 19

[Matt Brubeck (:mbrubeck)]

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.

Comment 20

[Jim Mathies [:jimm]]

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

Comment 21

[Jim Mathies [:jimm]]

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.

Comment 22

[Jim Mathies [:jimm]]

Attachment: subframe scroll using BrowserElementPanning v.2

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.

Comment 23

[Matt Brubeck (:mbrubeck)]

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.

Comment 24

[Jim Mathies [:jimm]]

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

Comment 25

[Jim Mathies [:jimm]]

Attachment: final patch

- updated per comments.

Comment 26

[Jim Mathies [:jimm]]

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

Comment 27

[Jim Mathies [:jimm]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Allison Naaktgeboren :ally]

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

Comment 30

[Jim Mathies [:jimm]]

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