937053 - tap() method should calculate elementInView from the coordinates of the tap

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[[:AndreiH]]

There is a new menu item in Settings that has pushed Bluetooth off the screen.
Bluetooth menu item is still visible a little bit so Marionette takes it as visible but doesn't/can't tap on it.

This is the test case that we are using:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_bluetooth.py

This is the method where we had to use scrollIntoView hack to resolve this:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/settings/app.py#L70

Comment 1

[Zac C (:zac)]

This causes us to put lots of hacks where elements are straddling the viewport.

Comment 2

[Zac C (:zac)]

Or alternatively Marionette could calculate the coordinate of the tap from dimentions of the element that is within the viewport and then not scroll at all.

Comment 5

[Zac C (:zac)]

This would still be a very valuable enhancement as we have loads of hacks working around it.

This is valid for all Marionette instances but is just much more apparent on the small viewport of mobile.

Comment 7

[Florin Strugariu [:Bebe]]

It looks like we have issues here with the way we calculate if the element is in view.

http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#790

Comment 8

[Zac C (:zac)]

Bebe, I thought we could pass the coordinates from inside tap into checkVisible, eg:

let c = coordinates(el, msg.json.corx, msg.json.cory);
if(checkVisible(el, c){ blah blah}

Which would inturn optionally pass the coordinate into elementInViewport. The coordinate position could be the rect which would mean the logic there does not need to change.

Comment 9

[Florin Strugariu [:Bebe]]

elementInViewport now checks if any part of the element is in the Viewport.

Because of that we have issues with this error.

I would think that the elementInViewport should return if the element is fully in the view port.

Is this correct?

Comment 10

[Zac C (:zac)]

An element can be wider than the viewport too so in that case it would be a false negative.

Comment 11

[Florin Strugariu [:Bebe]]

you are right zac I will update as you suggested

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Florin Strugariu [:Bebe] from comment #9)
> elementInViewport now checks if any part of the element is in the Viewport.
> 
> Because of that we have issues with this error.
> 
> I would think that the elementInViewport should return if the element is
> fully in the view port.
> 
> Is this correct?

So, elementInViewport is used only to check if we need to scroll to the element. It's possible the element will be too big to fit in the viewport, so we won't check if the element is fully inside the viewport. We just want to make sure that some part of it is within the viewport so we can interact with it. 

Maybe what we should do is scroll the center of the element into view, since tap() without any parameters will try to tap that area. This bug is likely because we say the Bluetooth element is in view because part of it is visible, but when we try to tap it, it's out of the viewport?

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

I'll wait for dburn's comments

Comment 14

[Zac C (:zac)]

(In reply to Malini Das [:mdas] from comment #12)
> This bug is likely
> because we say the Bluetooth element is in view because part of it is
> visible, but when we try to tap it, it's out of the viewport?

Exactly, yes.

but instead of assuming centre, we can pass in `c` that is created in the tap method. Then it covers default and bespoke coordinates cases.

Comment 15

[Florin Strugariu [:Bebe]]

Attachment: 0001-Bug-937053-tap-method-should-calculate-elementInView.patch

Comment 16

[David Burns :automatedtester]

Comment on attachment 8479096 [details] [diff] [review]
0001-Bug-937053-tap-method-should-calculate-elementInView.patch

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

::: testing/marionette/marionette-listener.js
@@ +783,5 @@
>   */
> +function elementInViewport(el, c) {
> +
> +  let rect = {};
> +  if (c){

Reading the code conditional, can we also change the name to be something meaningful since I am only guessing that c is coordinates, will always be invoked so the rect call below is not needed

@@ +787,5 @@
> +  if (c){
> +    rect = {
> +      top: c.y,
> +      left: c.x,
> +      bottom:c.y,

why is top and bottom the same value and why is right and left the same value?

Comment 17

[David Burns :automatedtester]

I am happy with Malini's idea in comment 12

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

There's two parts to this bug, the first is that elementInView should take into account the tap/click coordinates, defaulting to the center of the element. The second part is making the scroll into view also consider the coordinates - at the moment we use scrollIntoView(false), which aligns the element with the bottom of the scroll area. If the element happens to be vastly larger than the scroll area then this could still mean the click coordinates are not in view.

I'll work on the first part, which I suspect will fix most instances of this issue, and I'll probably leave the second part to bug 1003682, which already has a failing testcase as described.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Make sure point of interaction is in view before clicking/tapping. v1.0

Just requesting feedback at this point. This patch uses the offset coordinates specified (defaulting to center of element) to ensure the interaction point is in view. This changes the elementInViewport considerably, which previously returned true if any part of the element was in view. As far as I can tell this is only called in checkVisible, which itself is only called in clickElement, singleTap, and sendKeysToElement.

I was surprised that nothing else calls this (should the action chains not also call this?) and also was surprised that clickElement does not accept offset coordinates. Perhaps I'm missing something?

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8504836 [details] [diff] [review]
Make sure point of interaction is in view before clicking/tapping. v1.0

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

Looks like you're on the right track!

nit: element_outsite_viewport.html  should be element_outside_viewport.html

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8504836 [details] [diff] [review]
Make sure point of interaction is in view before clicking/tapping. v1.0

David: I'd appreciate your feedback here too. Thanks!

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Make sure point of interaction is in view before clicking/tapping. v1.1

My previous patch had a couple of issues, this one works fine on B2G and desktop Firefox. I had to write a custom failing test case though as the bluetooth one was passing for me, likely due to the larger screen on the Flame or a UI change.

Here's my failing test case for B2G, which this patch fixes:

from marionette import Marionette
m = Marionette()
m.start_session()

from gaiatest import GaiaApps
apps = GaiaApps(m)
apps.launch('Settings')

m.execute_script('arguments[0].scrollTop = 115;', script_args=[m.find_element('css selector', '#root div')])
m.find_element('id', 'menuItem-internetSharing').tap()

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=838d2a8cd269

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Make sure point of interaction is in view before clicking/tapping. v1.2

You didn't see that last patch, k?

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=065868da6463

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8505567 [details] [diff] [review]
Make sure point of interaction is in view before clicking/tapping. v1.2

Looks like the new test is failing on desktop Firefox. I'll look into this today.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I can't replicate this failure locally on OS X 10.9.5. Going to see if I can replicate it on my Ubuntu VM.

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Okay, I've replicate this and after much debugging I think I know what's going on. It appears that the target coordinates are beneath the scroll bar and for some reason this is reporting an inside the viewport. We then click the scroll bar, causing us to scroll horizontally!

I have resolved this by increasing the boundaries I'm testing to they're outside of even the scroll bar, but I'm concerned that our viewport calculations are not excluding the areas taken up by the scroll bars. Do either of you have any suggestions for how to proceed?

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Make sure point of interaction is in view before clicking/tapping. v1.3

In the meantime, here's an updated patch that avoids this problem, along with a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e8454d2c10ba

Comment 29

[David Burns :automatedtester]

I don't have any suggestions for this, is this purely on OSX? I wonder if the we put in a sleep of sorts so that the scroll bar disappears. It seems to be there for a small time after scrolling

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to David Burns :automatedtester from comment #29)
> I don't have any suggestions for this, is this purely on OSX? I wonder if
> the we put in a sleep of sorts so that the scroll bar disappears. It seems
> to be there for a small time after scrolling

No, it happens on my Ubuntu VM but not on my OSX 10.9.5. On try it failed for all desktop platforms. I don't think a sleep will help.

Comment 31

[David Burns :automatedtester]

we are relying on Gecko doing the right thing so perhaps there is a gecko issue here. I suggest speaking someone like roc/bz about this and perhaps they can redirect your question.

Does seem odd

Comment 32

[Malini Das [:mdas] - Away, not checking bugmail]

Yup, I second what dburns said, we should verify gecko behaviour here. Seems like a bug...

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8506275 [details] [diff] [review]
Make sure point of interaction is in view before clicking/tapping. v1.3

Okay, I'll spend a little bit of time on this next week and will raise a separate bug for the edge case when we have scroll bars and want to interact with an element at coordinates that straddle this area. I don't think it should block us from landing this patch though. Could I get a final review (and land or mark as checkin-needed if it passes)?

Comment 34

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8506275 [details] [diff] [review]
Make sure point of interaction is in view before clicking/tapping. v1.3

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

lgtm, thanks!

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e996cc695eb

Comment 36

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=313e5dcdfcb8 since one of this changes caused test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3160245&repo=mozilla-inbound

Comment 37

[Carsten Book [:Tomcat]]

re-checked in again since it was the other patch :)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/173d2c67a555

Comment 38

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to David Burns :automatedtester from comment #31)
> we are relying on Gecko doing the right thing so perhaps there is a gecko
> issue here. I suggest speaking someone like roc/bz about this and perhaps
> they can redirect your question.

It's not a gecko bug as we're using innerHeight/innerWidth, which include the scrollbar by design. I've raised bug 1086568 to fix Marionette so we calculate these excluding any scrollbars when determining if an element's interaction point is visible.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/173d2c67a555