Closed Bug 932804 Opened 6 years ago Closed 4 years ago

Compensate for statusbar in gaia apps

Categories

(Testing :: Marionette, defect, P1, major)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: viorela, Assigned: mdas)

References

Details

(Whiteboard: [xfail][u=marionette-user c=marionette-server p=3][touch][affects=b2g])

Attachments

(2 files, 2 obsolete files)

What error do you get when marionette tries to tap the select all button?

Does this work if you tap it manually?
No error. The test doesn't fail, but it doesn't tap the Select all button either. It works manually.
Hardware: All → x86_64
OS: All → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Whiteboard: [
Whiteboard: [ → [xfail]
It looks like there are many elements on this screen that cannot be tapped. We also cannot tap on the checkbox [1], and not even the Import button [2].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/import.html#L100-L111
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/import.html#L73
Setting major due to test blockage and to be picked up for a-team's q1 goals.
Severity: normal → major
Could we work around this by using Actions as in bug 862156?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(viorela.ioia)
I tried to replace the tap here with Marionette Actions, but still not working. The button is diplayed, but it can't be manipulated. I also used Actions with the checkbox element, and I got the same result.
Flags: needinfo?(viorela.ioia)
Whiteboard: [xfail] → [xfail][u=marionette-user c=marionette-server p=3]
Whiteboard: [xfail][u=marionette-user c=marionette-server p=3] → [xfail][u=marionette-user c=marionette-server p=3][touch]
Priority: -- → P2
Blocks: 1000037
Priority: P2 → P1
Assignee: nobody → mdas
I'll write down my notes here so I don't lose them. So if you use click() on the element, it's fine and that works, but not utils.synthesizeMouseAtCenter, nor our dispatching of mousemove/mousedown/mouseup.

trusted events don't matter: you can use either trusted events (finger on screen) or not trusted events (click()), and they both work on the button.

We are sending to the right coordinates with the button as the target, so I'm not yet sure where the problem is.
any event listeners on the window don't actually get fired when using tap() in this case, so that's a good lead.
https://hg.mozilla.org/mozilla-central/rev/d74281e6d571
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
I tagged that patch with the wrong bug. That commit fixes Bug 1001454.

This bug is still active.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is pretty weird. If I wire the touch events to use injectTouchEvent like in the APZ handler, then the buttons get touched, but no 'click' event is fired. So the buttons get enabled/disabled when I call tap(), but their click handlers don't get called back.
I finally narrowed it down to the problems:

1) We need to use injectTouchEvent from the root process (I don't have an explanation for why yet)

and
 
2) injectTouchEvent only uses coordinates to dispatch its events and the coordinate we use come from the iframes. The problem with this is that the coordinates therefore don't account for the displacement caused by the statusbar on the top of the screen. Therefore, all our of touch events dispatched by injectTouchEvent have been displaced, and now have to be modified by the height of the statusbar.

Davehunt, AutomatedTester, I'd like your thoughts the following:

for 2), The easiest way to solve it is to add a hardcode check in the marionette server. Adding it in the server is "bad practice" since it's gaia specific and is liable to break if 'statusbar' is no longer the id of the statusbar, but it is the easiest, and APZ stuff is already gaia specific...

If we want to move it over to the client side then the least painful way I can think of to do this is to have a function in marionette client itself called "implicitOffset(x,y)" which will implicitly add the given offset to any touch events you dispatch (tap(), or any Actions). Then, gaiatest will have to know when to apply the offset, ie: it will have to know when its in a child APZ frame. We can have a marionette server command called "inChildAPZFrame" which will return true if we're in a child APZ frame. So we can do this in gaiatest, whenver we have to do a switch to frame:

if self.marionette.in_child_apz_frame():
    self.marionette.implicit_offset(0, statusbar_height)
else:
    self.marionette.clear_implicit_offset()

so every tap/Action after this will be shifted appropriately if need be.

FYI, even if 1) is a legitimate bug, ie: even if we don't have to use injectTouchEvent for this particular case, 2) is still a legitimate bug because we're currently dispatching our apz scroll touches to the wrong coordinates.
Flags: needinfo?(dburns)
Flags: needinfo?(dave.hunt)
It's late and I realize my explanation of the taskbar displacement may not be clear.

To be clear about the shift, I mean we have to shift the y coordinate by 24 pixels (24 is the current height of the statusbar on my phone).

Actions(m).press(select_all, x, -24).wait(0.4).release().perform()

Just doing:
Actions(m).press(select_all).wait(0.4).release().perform()

totally misses the target element.
Heh, copy-pasted the wrong line:

(In reply to Malini Das [:mdas] from comment #13)
> Actions(m).press(select_all, x, -24).wait(0.4).release().perform()

shouldn't have a minus since we're adding 24 to the y coordinate:

Actions(m).press(select_all, x, 24).wait(0.4).release().perform()
(In reply to Malini Das [:mdas] from comment #12)
> I finally narrowed it down to the problems:
> 
> 1) We need to use injectTouchEvent from the root process (I don't have an
> explanation for why yet)
> 
> and
>  
> 2) injectTouchEvent only uses coordinates to dispatch its events and the
> coordinate we use come from the iframes. The problem with this is that the
> coordinates therefore don't account for the displacement caused by the
> statusbar on the top of the screen. Therefore, all our of touch events
> dispatched by injectTouchEvent have been displaced, and now have to be
> modified by the height of the statusbar.
> 
> Davehunt, AutomatedTester, I'd like your thoughts the following:
> 
> for 2), The easiest way to solve it is to add a hardcode check in the
> marionette server. Adding it in the server is "bad practice" since it's gaia
> specific and is liable to break if 'statusbar' is no longer the id of the
> statusbar, but it is the easiest, and APZ stuff is already gaia specific...
> 

+1 for Server side, I am happy for it to be added as a hack. Ideally if we can find out size of the status bar via get getBoundingClientRect() and then do the necessary instead of hardcoding the magic number

> If we want to move it over to the client side then the least painful way I
> can think of to do this is to have a function in marionette client itself
> called "implicitOffset(x,y)" which will implicitly add the given offset to
> any touch events you dispatch (tap(), or any Actions). 

-1 moving this over to the client side. We should take the coordinates given on the client side and manipulate on the server side. The reason being is that we will need to update all client bindings to make this work (marionette-js, marionette_client and any others out there)
Flags: needinfo?(dburns)
Note this statusbar height might change; it did just 2 months ago from 20 -> 24.
I agree that this would be better in the server side. Is there anything in addition to the status bar that would require a similar offset? Does this mean if the id of the status bar changed we'd need to update marionette? It's not ideal, but we would absolutely want a test that would promptly break on TBPL in this scenario. Is there also a way to fail in the test if we can identify that the co-ordinates intended to be hit are not those actually hit? This might help us to understand similar issues as they come up.
Flags: needinfo?(dave.hunt)
(In reply to David Burns :automatedtester from comment #15)
> (In reply to Malini Das [:mdas] from comment #12)
> > I finally narrowed it down to the problems:
> > 
> > 1) We need to use injectTouchEvent from the root process (I don't have an
> > explanation for why yet)
> > 
> > and
> >  
> > 2) injectTouchEvent only uses coordinates to dispatch its events and the
> > coordinate we use come from the iframes. The problem with this is that the
> > coordinates therefore don't account for the displacement caused by the
> > statusbar on the top of the screen. Therefore, all our of touch events
> > dispatched by injectTouchEvent have been displaced, and now have to be
> > modified by the height of the statusbar.
> > 
> > Davehunt, AutomatedTester, I'd like your thoughts the following:
> > 
> > for 2), The easiest way to solve it is to add a hardcode check in the
> > marionette server. Adding it in the server is "bad practice" since it's gaia
> > specific and is liable to break if 'statusbar' is no longer the id of the
> > statusbar, but it is the easiest, and APZ stuff is already gaia specific...
> > 
> 
> +1 for Server side, I am happy for it to be added as a hack. Ideally if we
> can find out size of the status bar via get getBoundingClientRect() and then
> do the necessary instead of hardcoding the magic number
>  
Absolutely, I was using getBoundingClientRect().bottom to get the height. The only hardcoded value would be the id of the statusbar so we can locate it.
> > If we want to move it over to the client side then the least painful way I
> > can think of to do this is to have a function in marionette client itself
> > called "implicitOffset(x,y)" which will implicitly add the given offset to
> > any touch events you dispatch (tap(), or any Actions). 
> 
> -1 moving this over to the client side. We should take the coordinates given
> on the client side and manipulate on the server side. The reason being is
> that we will need to update all client bindings to make this work
> (marionette-js, marionette_client and any others out there)
Good! :)
(In reply to Dave Hunt (:davehunt) from comment #17)
> I agree that this would be better in the server side. Is there anything in
> addition to the status bar that would require a similar offset? Does this
> mean if the id of the status bar changed we'd need to update marionette?

yes, unless there's a better way to figure out the offset or locate the statusbar

> It's not ideal, but we would absolutely want a test that would promptly
> break on TBPL in this scenario. Is there also a way to fail in the test if
> we can identify that the co-ordinates intended to be hit are not those
> actually hit? This might help us to understand similar issues as they come
> up.

I should be able to write a gaia-ui-test for this, and can have event listeners check the dispatched coordinates.

Davehunt, Zac, we'll need to have a test APZ app in Gaia, do you know if there is there one available?
(In reply to Malini Das [:mdas] from comment #19)
> Davehunt, Zac, we'll need to have a test APZ app in Gaia, do you know if
> there is there one available?

If not, writing a test that runs in TBPL to verify that we're compensating for the statusbar will be difficult. If such an app doesn't exist, perhaps I can ask to get one added to the builds of gaia used in testing
I know that Music, Gallery, and Video use APZ because all the scrolling tests broke when it landed. It might be worth having a dedicated test app though, similar to Template but with APZ enabled. Any ideas, Zac?
Flags: needinfo?(zcampbell)
In the "UI tests" app there are already a couple of page that load full screen and new windows and dialogs in various states, might one of those suffices?
Or place a new feature into that app.

It's a fairly stable app and you can rely on it being in the Engineering builds.
Flags: needinfo?(zcampbell)
(In reply to Zac C (:zac) from comment #22)
> In the "UI tests" app there are already a couple of page that load full
> screen and new windows and dialogs in various states, might one of those
> suffices?
> Or place a new feature into that app.
> 
> It's a fairly stable app and you can rely on it being in the Engineering
> builds.

Great, thanks, Zac.

Then to summarize, I'll fix the issue on the server side and add the gaia-ui-test for it, and add an app for it if necessary.
(In reply to Malini Das [:mdas] from comment #23)
> Then to summarize, I'll fix the issue on the server side and add the
> gaia-ui-test for it, and add an app for it if necessary.

This is a Marionette test, not a Gaia test. Can't you put it in the Marionette test repo?
(In reply to Zac C (:zac) from comment #24)
> (In reply to Malini Das [:mdas] from comment #23)
> > Then to summarize, I'll fix the issue on the server side and add the
> > gaia-ui-test for it, and add an app for it if necessary.
> 
> This is a Marionette test, not a Gaia test. Can't you put it in the
> Marionette test repo?

The reason I was leaning toward putting it in Gaia-ui-tests is because the the test in question will make sure that we compensate for the 'statusbar' if we're in Gaia. The test would use Gaia-ui-tests to navigate to an APZ app and check the functionality we're adding. It made sense to me to add it as part of Gu since we don't have a mechanism to test this in the marionette unit tests. 

We could add that mechanism if you prefer. In order for us to run this test as a marionette unit test, we'd have to create a new TBPL test job, say MnG for "marionette-gaia" unit tests which would test any assumptions marionette makes for gaia and would run against a b2g environment with Gaia.
Attached patch use statusbar offset (obsolete) — Splinter Review
I'll park my patch here and will r? when we settle on how to test it
I think that unit tests that require Gaia and use gaia-ui-test functionality should be in gaia-ui-tests.   If we want to split out gaia-ui-test unit tests into a separate job, we may, but that's orthogonal to this bug.

I don't think it makes sense to have a separate job that only contains marionette unit tests that happen to require gaia-ui-test functionality.
(In reply to Jonathan Griffin (:jgriffin) from comment #27)

> I don't think it makes sense to have a separate job that only contains
> marionette unit tests that happen to require gaia-ui-test functionality.

Well wouldn't you just run the Mn tests but against a desktopb2g instance? Then this test would be in that suite, but tagged to only run on b2g.
Arguably Marionette is developed for Firefox OS so it should have some kind of unit test framework, like WebDriver tests against all browser.

And also if you put it into Gu there's no guarantee it'll be maintained properly, it'll fall in the badlands of Gaia jurisdiction.
Mn tests don't use gaia-ui-test, and this unit test needs to.  So we can't just run Mn tests against desktopb2g to get this 'for free'.
Attachment #8421951 - Flags: review?(dburns)
As an update, I wrote the attached test to attempt to replicate this issue, but it fails to replicate the problem.

This test opens TestContainer, ensures it's an APZ app, switches into its child iframe, adds a button to it, taps it and verifies that it was tapped.

The problem is that this test passes without my patch, so it doesn't replicate the same problem we see in the gmail app. I'm not sure what makes the gmail environment different.

Since I'm heading out for a week and this investigation may take quite some time, I requested an r? on my server patch so we can at least get that landed, and get the test case for it landed afterward.
Comment on attachment 8421951 [details] [diff] [review]
use statusbar offset

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

we need to just try check the element that is returned, I think its dangerous to assume that it will be there.

::: testing/marionette/marionette-listener.js
@@ +119,5 @@
>    let tabParent = iframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.tabParent;
> +  //NOTE: The coordinates we receive are from the internal iframe, below the statusbar in Gaia.
> +  // Therefore, we have to offset the Y coordinate by the height of the statusbar
> +  let statusbar = curFrame.document.getElementById("statusbar");
> +  offsetY = message.clientY + statusbar.getBoundingClientRect().bottom;

if statusbar is null we are going to crash marionette here, we should only add the bottom value if we find it
Attachment #8421951 - Flags: review?(dburns) → review-
Heh, I just applied this patch after the new gaia changes, and it doesn't tap the button any more. Back to the drawing board.
(In reply to Malini Das [:mdas] from comment #32)
> Heh, I just applied this patch after the new gaia changes, and it doesn't
> tap the button any more. Back to the drawing board.

If I close Contacts and relaunch it, then tap works.

I have no explanation.
This issue is happening on FTU app too, when trying to tap the geolocation checkbox.
Malini, can you try this test case, please: https://pastebin.mozilla.org/5321795.
Flags: needinfo?(mdas)
(In reply to Viorela Ioia [:viorela] from comment #34)
> This issue is happening on FTU app too, when trying to tap the geolocation
> checkbox.
> Malini, can you try this test case, please:
> https://pastebin.mozilla.org/5321795.

To get back to you, I can confirm the button isn't being pressed, but since I have to update my patch, I can't tell if this patch will fix it or not, or if this is a different issue. I'll give you an update when I know the cause
Flags: needinfo?(mdas)
Mdas, suspect it will work because we worked around this bug exactly by putting .tap(y=20) to offset the tap and the test passed :)
Attached patch use statusbar offset v2.0 (obsolete) — Splinter Review
Oh jeez. This patch was a bit of a nightmare.

So I had to ensure the Touch object we create accounts for the offset. Since a) the Touch object's clientY is immutable after creation and b) I can't share access to objects across content frames and c)I can't retrieve Touch objects based on their ID using any method known to gecko, I instead moved all Touch object create to emitTouchEvent.

Next, I had to retrieve the statusbar height from the root frame from within the childframe. Since content to content message passing isn't supported (grr!), I made the server hold the height of the statusbar.

Lastly, we use clientHeight of the statusbar because even when the statusbar isn't visible, like in the FTU app, the statusbar still affects the position! We had to shift even though its actual height was 0 and it doesn't appear on the page. Gaia is fun.

Here are my try attempts:

Mn: https://tbpl.mozilla.org/?tree=Try&rev=16412934906a
Mn, Mnw:  https://tbpl.mozilla.org/?tree=Try&rev=6457d3f83f17
Gu: https://tbpl.mozilla.org/?tree=Try&rev=eabeb2ff795c
Attachment #8421951 - Attachment is obsolete: true
will r? when they pass
Comment on attachment 8435824 [details] [diff] [review]
use statusbar offset v2.1

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

try's green!
Attachment #8435824 - Flags: review?(dburns)
Attachment #8435824 - Flags: review?(dburns) → review+
leave open, will need to uplift to 1.4
Keywords: leave-open
And Aurora presumably (comment 41 missed the 32 train).
Target Milestone: mozilla32 → ---
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #44)
> https://hg.mozilla.org/mozilla-central/rev/a8ad10daa7c0

Backed out from m-c at mdas' request, to fix the jenkins builds (that use the images from m-c):
remote:   https://hg.mozilla.org/mozilla-central/rev/13be61241671
Apparently this change was responsible for bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=1023289, but I am having trouble reproducing it locally so far...
great, /gaiatest/tests/functional/browser/test_browser_tabs.py is reproducible locally
Depends on: 1023251
Whiteboard: [xfail][u=marionette-user c=marionette-server p=3][touch] → [xfail][u=marionette-user c=marionette-server p=3][touch][affects=webqa]
Summary: Tapping on select all button doesn't work on Gmail or Outlook frame, before importing contacts → Compensate for statusbar in gaia apps
Just wondering, what will happen if we want to tap on the status bar? Will Marionette still be able to do that?
(In reply to Zac C (:zac) from comment #49)
> Just wondering, what will happen if we want to tap on the status bar? Will
> Marionette still be able to do that?

Yup, this compensation code only applies to APZ child frames. Since the statusbar is in the main process, we won't add any compensation
As an update:

There were issues being conflated when I was trying to figure out the bugs caused by this patch:

1) modifying createATouch so that it gets dispatched with the correct x,y coordinates for APZ touch fixed the gmail test if no-restart happened, but had no positive effect if we did restart.
2) I get "tabparent is null" errors in some apps when doing APZ touch after modifying the createATouch code.
3) after reboot, we let the user connect to marionette too soon, so the statubar isn't available, and so we don't get a height to compensate with, so it explains why 1) happened, ie: we didn't compensate for the statusbar since we got a null height.

I have a patch that fixes all of the above, and passes almost all the tests... except it appears to generate duplicate touch events only when running test_clock_create_new_alarm.py. It could be that I'm not removing a listener somewhere. I'll be working on this.
Whiteboard: [xfail][u=marionette-user c=marionette-server p=3][touch][affects=webqa] → [xfail][u=marionette-user c=marionette-server p=3][touch][affects=b2g]
Closing as these bugs are not currently relevant. We can reopen if the issue happens on Fennec.
Status: REOPENED → RESOLVED
Closed: 6 years ago4 years ago
Resolution: --- → WONTFIX
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.