Closed Bug 901607 Opened 7 years ago Closed 7 years ago

Add front end sub-frame scroll support

Categories

(Firefox for Metro Graveyard :: Browser, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jimm, Unassigned)

References

()

Details

Attachments

(2 files, 5 obsolete files)

> 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
Whiteboard: [preview]
Assignee: nobody → jmathies
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.
Attachment #789059 - Flags: review?(netzen)
Comment on attachment 789059 [details] [diff] [review]
subframe scroll using BrowserElementPanning

just noticed some unused code in the panning module.
Attachment #789059 - Flags: review?(netzen)
Attachment #789059 - Attachment is obsolete: true
Attachment #789067 - Flags: review?(netzen)
Could you rebase this after first applying the patches for bug 898580 and bug 895905?  botond is close to landing his patch.
Attachment #789067 - Flags: review?(netzen)
Depends on: 898580
Hmm. Bug 906877 will break this patch. Perhaps we should hold off on landing that until Metro switches over to APZC completely.
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? :)
Depends on: 906877
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.
Blocks: 892478
Blocks: 906043
Updated to work with the patch in bug 906877.
Attachment #789067 - Attachment is obsolete: true
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.
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.
(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.
This is waiting on kats landing in the parent bug.
Attachment #794330 - Attachment is obsolete: true
Attachment #794771 - Flags: review?(mbrubeck)
(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?
No longer blocks: 892478
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.
Attachment #794771 - Flags: review?(mbrubeck) → review-
(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."
Updated per comments. Nixed the active logic, view change events, and zoom logic.
Attachment #794771 - Attachment is obsolete: true
Attachment #795606 - Flags: review?(mbrubeck)
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 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.
Attachment #795606 - Flags: review?(mbrubeck) → review-
(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.
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.
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.
Attachment #795606 - Attachment is obsolete: true
Attachment #796111 - Flags: review?(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.
Attachment #796111 - Flags: review?(mbrubeck) → review+
(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.
Whiteboard: [preview] → [preview][waiting on bug 906877]
Attached patch final patchSplinter Review
- updated per comments.
My mistake, no dependency anymore, since I'm not touching bep.
No longer depends on: 906877
Whiteboard: [preview][waiting on bug 906877] → [preview]
> ::: 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?
(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.
Assignee: jmathies → nobody
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Whiteboard: [preview]
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.