Unable to automate scrolling in apps with APZ enabled

RESOLVED FIXED in Firefox OS v1.4

Status

Testing
Marionette
P1
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: davehunt, Assigned: mdas)

Tracking

({ateam-marionette-goal, ateam-marionette-userinput})

unspecified
mozilla31
ARM
Gonk (Firefox OS)
ateam-marionette-goal, ateam-marionette-userinput
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.4 fixed)

Details

(Whiteboard: [touch])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Since bug 942726 the b2gperf tests have been unable to scroll in certain applications. Those affected appear to be Gallery, Music, and Video.
(Reporter)

Comment 1

4 years ago
This is also affecting Contacts, Messages, and Settings. They just aren't failing because there are still values for the FPS metric. The only apps not affected are Homescreen and Browser.
(Reporter)

Comment 2

4 years ago
The following reproduces the issue with APZ enabled:

1. Launch Contacts app
2. Run the following in an interactive Python shell:

> from marionette import Actions
> from marionette import Marionette
> from marionette.by import By
> m = Marionette()
> m.start_session()
> m.switch_to_frame(m.find_element(By.CSS_SELECTOR, 'iframe[src*="contacts"]'))
> contact = m.find_elements(By.CSS_SELECTOR, 'li.contact-item')[4]
> Actions(m).flick(contact, contact.size['width'] / 2, contact.size['height'] / 2, contact.size['width'] / 2, -100, 200).perform()
> m.delete_session()

Expected:
The contacts list should scroll 100 pixels.

Actual:
Nothing happens.

If you then disable APZ for apps by holding the power button and selecting the option, restart the Contacts app, and run the above script again, you should see it scroll.
Assignee: dave.hunt → nobody
Status: ASSIGNED → NEW
As I commented in a private email thread. Are you sending the events directly into the app process ? If yes you should send them to the main process instead since it is the one that is doing the async scrolling now.

Homescreen is not affected since it used it owns custom scrolling code. Browser already runs in the main process so the events should correctly be caught to be converted to a scrolling gesture.
Blocks: 909877
The Actions code executes it content so we will have to move it into the chrome space. 

Vivien since we only have 1 chrome object, is this going to cause problems? Asking so that I know what to expect

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#934
Flags: needinfo?(21)
Blocks: 949585
No longer blocks: 909877
(In reply to David Burns :automatedtester from comment #4)
> The Actions code executes it content so we will have to move it into the
> chrome space. 
> 
> Vivien since we only have 1 chrome object, is this going to cause problems?
> Asking so that I know what to expect
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js#934

I'm not really sure what you mean by that. If you mean that there is only one chrome object for all the apps instead of one chrome object per app it should not matter as APZC will look at the app which is currently displayed on screen and scroll it.

Let me know if I misunderstood the question.
Flags: needinfo?(21)
(Reporter)

Updated

4 years ago
Blocks: 959712
Component: Gaia → Marionette
Product: Firefox OS → Testing
Looks like we'll need to modify Marionette to deal with this; Malini, can you take a look?
Btw Vivien recently landed bug 959242 which be of some help here.
Assignee: nobody → mdas
Created attachment 8373576 [details] [diff] [review]
apz patch so far

I can't work on this right now, so I'm putting the current work up here for dburns to pick up :)

So, from speaking with kats, iframe's sendTouchEvent is we want to call. This patch hooks up a listener in the main content frame (named "MarionetteMainListener:emitTouchEvent"), and if an apz-enabled iframe wants to dispatch a touch event, it will send "Marionette:emitTouchEvent" to the marionette-server.js, which will relay the message using the global message manager to the main content frame using "MarionetteMainListener:emitTouchEvent" (it has to relay it because there is no OOP content frame to content frame communication using message managers. The global message manager is of no use in that world). MarionetteMainListener:emitTouchEvent calls the sendTouchEvent function on the correct iframe, but...

The problem is that sendTouchEvent doesn't seem to be defined:

E/GeckoConsole(  570): [JavaScript Error: "iframe.sendTouchEvent is not a function" {file: "chrome://marionette/content/marionette-listener.js" line: 124}]

The iframe object is of type: [object XrayWrapper [object HTMLIFrameElement], perhaps we need  to unwrap it?

I can confirm that APZ is enabled. We need to figure out why this isn't working, and how to get these events sent.
I have managed to get mdas' patch to no error but its not working...

I think the next step is to see what I can revive https://bugzilla.mozilla.org/show_bug.cgi?id=853622 since that, in my opinion, is the root of the issue
just so I don't lose it, zac created a test to populate the address book to help with testing

from gaiatest import GaiaTestCase
from gaiatest.mocks.mock_contact import MockContact
class AddContacts(GaiaTestCase):
   def test_add_heaps(self):
       for i in range(0, 200):
           self.data_layer.insert_contact(MockContact())
(Reporter)

Comment 11

4 years ago
(In reply to David Burns :automatedtester from comment #10)
> just so I don't lose it, zac created a test to populate the address book to
> help with testing
> 
> from gaiatest import GaiaTestCase
> from gaiatest.mocks.mock_contact import MockContact
> class AddContacts(GaiaTestCase):
>    def test_add_heaps(self):
>        for i in range(0, 200):
>            self.data_layer.insert_contact(MockContact())

FWIW, you can also use b2gpopulate [1], which adds a reference workload database with realistic data rather than using the API.

[1] https://pypi.python.org/pypi/b2gpopulate
No longer blocks: 949585
Blocks: 949585
bumping priority so we can look at this soon.
Severity: normal → major
Blocks: 968290
Created attachment 8395926 [details] [diff] [review]
Patch so far, from mdas', that doesnt error but still doesnt work
Whiteboard: [touch]
Priority: -- → P1
Created attachment 8402673 [details] [diff] [review]
working apz scroll

try for desktop firefox:
https://tbpl.mozilla.org/?tree=Try&rev=46d2c2aa93ab

try for b2g:
https://tbpl.mozilla.org/?tree=Try&rev=0fec27373b3b

Gu test:
https://tbpl.mozilla.org/?tree=Try&rev=831c66461931
Attachment #8373576 - Attachment is obsolete: true
Attachment #8395926 - Attachment is obsolete: true
Attachment #8402673 - Flags: review?(dburns)
Comment on attachment 8402673 [details] [diff] [review]
working apz scroll

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

I'm going to change the patch so it only uses injectTouchEvent when scrolling, not for every event.
Attachment #8402673 - Flags: review?(dburns)
Created attachment 8402794 [details] [diff] [review]
working apz scroll v2

This patch only uses injectTouchEvent when necessary, by looking ahead in the action chain.

desktop firefox/desktop-b2g:
https://tbpl.mozilla.org/?tree=Try&rev=ee64bd06585d

b2g:
https://tbpl.mozilla.org/?tree=Try&rev=0fec27373b3b
Attachment #8402673 - Attachment is obsolete: true
Attachment #8402794 - Flags: review?(dburns)
Comment on attachment 8402794 [details] [diff] [review]
working apz scroll v2

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

Commit message needs updating and a few minor nits.

only thing I was curious about why we can't do this for all touch events.

::: testing/marionette/marionette-listener.js
@@ +117,5 @@
> +  let message = message.json;
> +  let frames = curFrame.document.getElementsByTagName("iframe");
> +  let iframe = frames[message.index];
> +  let identifier = touchId = nextTouchId++;
> +  iframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.tabParent.injectTouchEvent(message.type, [identifier], [message.clientX], [message.clientY], [message.radiusX], [message.radiusY], [message.rotationAngle], [message.force], 1, 0);

Can we make this multiline

@@ +679,5 @@
>    if (!wasInterrupted()) {
>      let loggingInfo = "emitting Touch event of type " + type + " to element with id: " + touch.target.id + " and tag name: " + touch.target.tagName + " at coordinates (" + touch.clientX + ", " + touch.clientY + ") relative to the viewport";
>      dumpLog(loggingInfo);
> +    var docShell = curFrame.document.defaultView.
> +    QueryInterface(Components.interfaces.nsIInterfaceRequestor).

nit (and this could just be splinter), can we have this indented so it doesnt line up with the var above.

@@ +683,5 @@
> +    QueryInterface(Components.interfaces.nsIInterfaceRequestor).
> +    getInterface(Components.interfaces.nsIWebNavigation).
> +    QueryInterface(Components.interfaces.nsIDocShell);
> +    if (docShell.asyncPanZoomEnabled && scrolling) {
> +      // if we're in APZ and we're scrolling, we must use injectTouchEvent to dispatch our touchmove events 

extra white space

@@ +687,5 @@
> +      // if we're in APZ and we're scrolling, we must use injectTouchEvent to dispatch our touchmove events 
> +      let index = sendSyncMessage("MarionetteFrame:getCurrentFrameId");
> +      // only call emitTouchEventForIFrame if we're inside an iframe.
> +      if (index != null) {
> +        sendSyncMessage("Marionette:emitTouchEvent", {index: index, type: type, id: touch.identifier, clientX: touch.clientX, clientY: touch.clientY, radiusX: touch.radiusX, radiusY: touch.radiusY, rotation: touch.rotationAngle, force: touch.force});

can we make this multiline so its easier to read

@@ +973,4 @@
>          sendError("Invalid Command: press cannot follow an active touch event", 500, null, command_id);
>          return;
>        }
> +      // look ahead to check if we're scrolling. Needed for APZ touch dispatching.

Why can't we do this for all touch dispatching in the APZ world?
Attachment #8402794 - Flags: review?(dburns) → review+
(In reply to David Burns :automatedtester from comment #17)
> Why can't we do this for all touch dispatching in the APZ world?

We can, but is it needed? It seemed unnecessarily roundabout to use the message manager to dispatch every event using another frame instead of calling sendTouchEvent directly when possible, since injectTouchEvent is only needed when we're doing a scroll action. I can change it to using injectTouchEvent for everything if we're in apz if you prefer though, I'm not strongly opinionated about it.
s/is it needed/will it be needed

is what I meant to say. You don't need to use injectTouchEvent for anything other than scrolling right now in APZ apps.
I am not opinionated either but as long as its easy to diagnose if we have issues here later I am happy with what you want to do here
Created attachment 8404710 [details] [diff] [review]
working apz scroll v3

carrying r+ forward, and updated patch.

Landed in m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/679005d79780
Attachment #8402794 - Attachment is obsolete: true
Attachment #8404710 - Flags: review+
Keywords: ateam-marionette-goal, ateam-marionette-userinput
https://hg.mozilla.org/mozilla-central/rev/679005d79780
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Reporter)

Updated

4 years ago
Blocks: 1001454
(Reporter)

Comment 23

4 years ago
:mdas this is still failing against mozilla-b2g30_v1_4, could we get this and the related patches uplifted?
Flags: needinfo?(mdas)
sure I'll uplift patches for Bug 958036, Bug 1001454 and Bug 995989
Flags: needinfo?(mdas)
They've landed as 
   https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/14bc6e0d9814
   https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/fa1c1eebc121
   https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2cea7fbc4072
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.