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)
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)
(deleted),
patch
|
mdas
:
review+
|
Details | Diff | Splinter Review |
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•11 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•11 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
Comment 3•11 years ago
|
||
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: gaia-apzc
Comment 4•11 years ago
|
||
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)
Updated•11 years ago
|
Comment 5•11 years ago
|
||
(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)
Updated•11 years ago
|
Component: Gaia → Marionette
Product: Firefox OS → Testing
Comment 6•11 years ago
|
||
Looks like we'll need to modify Marionette to deal with this; Malini, can you take a look?
Comment 7•11 years ago
|
||
Btw Vivien recently landed bug 959242 which be of some help here.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mdas
Assignee | ||
Comment 8•11 years ago
|
||
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.
Comment 9•11 years ago
|
||
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
Comment 10•11 years ago
|
||
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•11 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
Updated•11 years ago
|
No longer blocks: gaia-apzc-2
Updated•11 years ago
|
Blocks: gaia-apzc-2
Assignee | ||
Comment 12•11 years ago
|
||
bumping priority so we can look at this soon.
Severity: normal → major
Comment 13•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [touch]
Updated•11 years ago
|
Priority: -- → P1
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
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.
Comment 20•11 years ago
|
||
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
Assignee | ||
Comment 21•11 years ago
|
||
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+
Updated•11 years ago
|
Keywords: ateam-marionette-goal,
ateam-marionette-userinput
Comment 22•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Reporter | ||
Comment 23•11 years ago
|
||
:mdas this is still failing against mozilla-b2g30_v1_4, could we get this and the related patches uplifted?
Flags: needinfo?(mdas)
Assignee | ||
Comment 24•11 years ago
|
||
Flags: needinfo?(mdas)
Assignee | ||
Comment 25•11 years ago
|
||
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
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•