938242 - remove visibility check in Actions

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Etienne Segonzac (:etienne)]

In bug 936585 we're testing edge gestures that are initiated on "invisible" panels on each side of the screen.

We have to enable the debug mode during the test (which gives the panels some opacity) in order to cover this with marionette tests. Otherwise we get a:

 ElementNotVisible: (11) Element is not currently visible and may not be manipulated

I wonder if there is a strong enough use case to add an option allowing the manipulation of invisible elements.

Comment 1

[Zac C (:zac)]

You could try touching on the top-most visible element but use pixel references to target the actual invisible panel section.

As the touch events come in at native/Gecko level it should work but you never know, it might not.

Comment 2

[Zac C (:zac)]

It works exactly as I described above in comment #1.
Here we have done it with Python tests
https://github.com/mozilla-b2g/gaia/pull/13697/files

However it does not work on desktopb2g owing to bug #891882

Comment 3

[David Burns :automatedtester]

Is Actions not good enough for this? Since they should be "do what I say" type API and should ignore visibility.

Comment 4

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

(In reply to David Burns :automatedtester from comment #3)
> Is Actions not good enough for this? Since they should be "do what I say"
> type API and should ignore visibility.

Actions check the visibility of the element you're pressing on: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#941

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Malini Das [:mdas] from comment #4) 
> Actions check the visibility of the element you're pressing on:
> https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> listener.js#941

The actions interfaces are meant to call down to the underlying mouse interaction layer.  It'll calculate coordinates, so if those can't be derived there's an error.  But once those are done, the click happens at a point, not on an element.  Hence the visibility check on the “underlying” element seems wrong.

Comment 6

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

(In reply to Andreas Tolfsen (:ato) from comment #5)
> (In reply to Malini Das [:mdas] from comment #4) 
> > Actions check the visibility of the element you're pressing on:
> > https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> > listener.js#941
> 
> The actions interfaces are meant to call down to the underlying mouse
> interaction layer.  It'll calculate coordinates, so if those can't be
> derived there's an error.  But once those are done, the click happens at a
> point, not on an element.  Hence the visibility check on the “underlying”
> element seems wrong.

Heh, I'm not saying that it's right, I'm just saying that's the current behaviour. We can change it since that makes more sense

Comment 7

[Zac C (:zac)]

There is a simple workaround anyway; just like I did in the Python version linked above.

I would not like the option to manipulate invisible elements. It would be open to abuse through lazy programming, result in poor UI testing and loads of false positives.

Comment 8

[David Burns :automatedtester]

(In reply to Andreas Tolfsen (:ato) from comment #5)
> (In reply to Malini Das [:mdas] from comment #4) 
> > Actions check the visibility of the element you're pressing on:
> > https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> > listener.js#941
> 
> The actions interfaces are meant to call down to the underlying mouse
> interaction layer.  It'll calculate coordinates, so if those can't be
> derived there's an error.  But once those are done, the click happens at a
> point, not on an element.  Hence the visibility check on the “underlying”
> element seems wrong.

This is correct, for actions we shouldnt call down to visibility (and I probably said it wrong in the first place, sorry). Actions is a "Do what I say" API 

For API elements hanging off webelement, so click(), send_keys() and tap() we should check visibility. WebElement API is " Do what I mean" API.

Hope that clarifies things.

Comment 9

[David Burns :automatedtester]

(In reply to Zac C (:zac) from comment #7)
> There is a simple workaround anyway; just like I did in the Python version
> linked above.
> 
> I would not like the option to manipulate invisible elements. It would be
> open to abuse through lazy programming, result in poor UI testing and loads
> of false positives.

So we arent going too add an option, we are just doing another alignment with WebDriver. The Actions API can abuse things anyway. For times where we are using actions to get around visibility bugs I would want us to document( read as raise bug depending on the visibility bug) so that the Right Way™ is then put in place

Comment 10

[David Burns :automatedtester]

pushed to try will check later

Comment 11

[David Burns :automatedtester]

Try is green

https://tbpl.mozilla.org/?tree=Try&rev=0868481e4cc7
https://tbpl.mozilla.org/?tree=Try&rev=929868dd4a03

Comment 12

[David Burns :automatedtester]

Attachment: Remove the visibility checks for touch actions in Marionette;

Comment 13

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

Comment on attachment 8339258 [details] [diff] [review]
Remove the visibility checks for touch actions in Marionette;

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

thanks!

Comment 14

[David Burns :automatedtester]

Landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/234a044ca942

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/234a044ca942

Comment 16

[Bob Silverberg [:bsilverberg]]

Can we get this into b2g26_v1.2 as well, so we can enable the music app tests there too?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/062ee6fe46df