Closed Bug 958036 Opened 11 years ago Closed 11 years ago

Unable to automate scrolling in apps with APZ enabled

Categories

(Remote Protocol :: Marionette, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g-v1.4 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: davehunt, Assigned: mdas)

References

Details

(Keywords: pi-marionette-goal, pi-marionette-userinput, Whiteboard: [touch])

Attachments

(1 file, 4 obsolete files)

Since bug 942726 the b2gperf tests have been unable to scroll in certain applications. Those affected appear to be Gallery, Music, and Video.
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.
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.
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)
(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)
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
Attached patch apz patch so far (obsolete) (deleted) — Splinter Review
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())
(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
bumping priority so we can look at this soon.
Severity: normal → major
Whiteboard: [touch]
Priority: -- → P1
Attached patch working apz scroll (obsolete) (deleted) — Splinter Review
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)
Attached patch working apz scroll v2 (obsolete) (deleted) — Splinter Review
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
Attached patch working apz scroll v3 (deleted) — Splinter Review
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+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1001454
: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)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: