Closed Bug 937053 Opened 6 years ago Closed 5 years ago

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

Categories

(Testing :: Marionette, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: AndreiH, Assigned: davehunt)

References

Details

(Whiteboard: [touch][affects=b2g])

Attachments

(1 file, 4 obsolete files)

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
Blocks: 865232
No longer depends on: 865232
Whiteboard: [touch]
This causes us to put lots of hacks where elements are straddling the viewport.
Blocks: 801898
Priority: -- → P1
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.
Whiteboard: [touch] → [touch][affects=webqa]
Duplicate of this bug: 875830
Duplicate of this bug: 877651
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.
Duplicate of this bug: 878017
Whiteboard: [touch][affects=webqa] → [touch][affects=b2g]
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
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.
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?
Flags: needinfo?(mdas)
Flags: needinfo?(dburns)
An element can be wider than the viewport too so in that case it would be a false negative.
you are right zac I will update as you suggested
(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?
Flags: needinfo?(mdas)
I'll wait for dburn's comments
(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 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?
I am happy with Malini's idea in comment 12
Flags: needinfo?(dburns)
Assignee: nobody → florin.strugariu
Attachment #8479096 - Flags: review?(mdas)
Attachment #8479096 - Flags: review?(dburns)
Attachment #8479096 - Flags: review?
Attachment #8479096 - Flags: review-
User Story: (updated)
User Story: (updated)
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.
Assignee: florin.strugariu → dave.hunt
Status: NEW → ASSIGNED
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?
Attachment #8479096 - Attachment is obsolete: true
Attachment #8479096 - Flags: review?
Attachment #8504836 - Flags: feedback?(mdas)
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
Attachment #8504836 - Flags: feedback?(mdas) → feedback+
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!
Attachment #8504836 - Flags: feedback?(dburns)
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()
Attachment #8504836 - Attachment is obsolete: true
Attachment #8504836 - Flags: feedback?(dburns)
Attachment #8505473 - Flags: review?(mdas)
Attachment #8505473 - Flags: review?(dburns)
You didn't see that last patch, k?

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=065868da6463
Attachment #8505473 - Attachment is obsolete: true
Attachment #8505473 - Flags: review?(mdas)
Attachment #8505473 - Flags: review?(dburns)
Attachment #8505567 - Flags: review?(mdas)
Attachment #8505567 - Flags: review?(dburns)
Attachment #8505567 - Flags: review?(dburns) → review+
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.
Attachment #8505567 - Flags: review?(mdas)
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.
Flags: needinfo?(dave.hunt)
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?
Flags: needinfo?(mdas)
Flags: needinfo?(dburns)
Flags: needinfo?(dave.hunt)
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
Attachment #8505567 - Attachment is obsolete: true
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
Flags: needinfo?(dburns)
(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.
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
Yup, I second what dburns said, we should verify gecko behaviour here. Seems like a bug...
Flags: needinfo?(mdas)
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)?
Attachment #8506275 - Flags: review?(mdas)
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!
Attachment #8506275 - Flags: review?(mdas) → review+
re-checked in again since it was the other patch :)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/173d2c67a555
Flags: needinfo?(dave.hunt)
(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.
https://hg.mozilla.org/mozilla-central/rev/173d2c67a555
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.