1155116 - [fxos-automation] marionette-js-client should have contextMenuClick() method to compliment other touch and click events

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

For integration test, we may need the following APIs:

1. to send keyboard event to "active" element
     Although we already have [1] to send key event, the target should be queried in advance. This doesn't make sense for TV case because the "active" element may change dynamically, like a popup overlay shows up and grabs focus. In our opinion, we may want to have a API to send key event to an app and let gecko to decide who should receive that key event.

2. to query "active" element (optional)
     We may need to test the focus lost issue. The "active" element is |document.activeElement|. We may use Element.scriptWith or other similar API to query it. But it looks so "inconvenient".

3. to change focus (optional)
     To simulate the test case, we may need to change the focus to a certain state. Like previous item, we may use existing API to do so. But it looks so "inconvenient".

[1] http://mozilla-b2g.github.io/marionette-js-client/api-docs/classes/Marionette.Element.html

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sorry, I forgot one.

4. to show context menu
     The context menu can be triggered by long press or mouse right click. If we use long press, we cannot make sure how long to press it can trigger context menu. If we use right click, there is no API to do so. For TV, we have a option button which binds to context menu. User may just press it and send a context menu event to current active element.

Comment 2

[Aus Lacroix [:aus]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)
> For integration test, we may need the following APIs:
> 
> 1. to send keyboard event to "active" element
>      Although we already have [1] to send key event, the target should be
> queried in advance. This doesn't make sense for TV case because the "active"
> element may change dynamically, like a popup overlay shows up and grabs
> focus. In our opinion, we may want to have a API to send key event to an app
> and let gecko to decide who should receive that key event.

I'm having trouble understanding why having to find the element and then sending the key event would be bad. Integration tests are supposed to have a deterministic path through the UI. If the path is variable but you expect the same outcome, then shouldn't you have a way to test each of those reliably?

If you really want to do this you can send the KeyboardEvent to the top-level app window using 'sendKeys' on Element. However, I feel like this shouldn't be necessary.  

> 2. to query "active" element (optional)
>      We may need to test the focus lost issue. The "active" element is
> |document.activeElement|. We may use Element.scriptWith or other similar API
> to query it. But it looks so "inconvenient".

I would suggest writing a helper that does the pre-determined steps required to set the element active (focus it or what not), then take focus away and query the attributes or what not that you want after that to make sure it handled the blur properly. Every UI is different so writing high level testing functions is not really possible in a generic manner.

I would suggest looking at app integration tests libraries that folks have written to accomplish such things. The email app has a particularly good model which most other apps use -- https://github.com/mozilla-b2g/gaia/blob/master/apps/email/test/marionette/lib/email.js , this wraps a lot of common interactions to help write the tests at a higher level.

> 3. to change focus (optional)
>      To simulate the test case, we may need to change the focus to a certain
> state. Like previous item, we may use existing API to do so. But it looks so
> "inconvenient".
> 
> [1]
> http://mozilla-b2g.github.io/marionette-js-client/api-docs/classes/
> Marionette.Element.html

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #2)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)
> > 1. to send keyboard event to "active" element
> 
> I'm having trouble understanding why having to find the element and then
> sending the key event would be bad. Integration tests are supposed to have a
> deterministic path through the UI. If the path is variable but you expect
> the same outcome, then shouldn't you have a way to test each of those
> reliably?

Ghislain,

Thanks for your suggestion. I had the same idea before but changed with the following reasons (please tell me if I am wrong):

1. IIRC, some QAs told me that it is more proper to write an integration test to simulate user's behavior. If a user uses keyboard, she may not need to know which one is focused element, but she may know the top-most UI should be closed while she press "OK" or "Back" keys which are default keys in TV. To simulate this behavior, we want to send "OK" or "Back" keys to the focused element without knowing which one is the active element.

2. If there is any events occurred, like a notification shows up, system app switches active element to notification. And if this happens between finding the element, which should be the active element but not after event got, and sending keys to that element, the test case will pass because the element we found behaves correctly. But in real case, user still have the UI because the key is send to notification but not this UI. 


The above use case implies two things:

i. the focus should be correct
ii. the behavior of keyevent handler should be correct

We can separate them as two test cases, but these test cases don't like user behaviors. We can merge them as one test case, but it needs a way to prevent the unexpected change of active element.

> 
> If you really want to do this you can send the KeyboardEvent to the
> top-level app window using 'sendKeys' on Element. However, I feel like this
> shouldn't be necessary.

If I am wrong at previous part, we don't need to discuss this part.

May we use 'findElement' to query window object? If possible, I will try it.

Does sendsKeys of Element use 'synthesizeKey'? I had asked the same question at Taipei gecko sharing. They told me that synthesizeKey can send keyboard event to active element if we send an event to iframe's contentWindow.

> 
> > 2. to query "active" element (optional)
> >      We may need to test the focus lost issue. The "active" element is
> > |document.activeElement|. We may use Element.scriptWith or other similar API
> > to query it. But it looks so "inconvenient".
> 
> I would suggest writing a helper that does the pre-determined steps required
> to set the element active (focus it or what not), then take focus away and
> query the attributes or what not that you want after that to make sure it
> handled the blur properly. Every UI is different so writing high level
> testing functions is not really possible in a generic manner.
> 
> I would suggest looking at app integration tests libraries that folks have
> written to accomplish such things. The email app has a particularly good
> model which most other apps use --
> https://github.com/mozilla-b2g/gaia/blob/master/apps/email/test/marionette/
> lib/email.js , this wraps a lot of common interactions to help write the
> tests at a higher level.

Thanks for this one.


(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #1)
> 4. to show context menu
>      The context menu can be triggered by long press or mouse right click.
> If we use long press, we cannot make sure how long to press it can trigger
> context menu. If we use right click, there is no API to do so. For TV, we
> have a option button which binds to context menu. User may just press it and
> send a context menu event to current active element.

BTW, it looks like that we have context_click[1] in python marionette. May we have the same API in js marionette?

Thanks.

[1] https://marionette-client.readthedocs.org/en/latest/reference.html#marionette_driver.marionette.Actions.context_click

Comment 4

[Aus Lacroix [:aus]]

John, I'm looking into the context menu click handling, I should have news soon. I'm leaving the NI? flag set so I don't forget to update the bug.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thank you Ghislain~~

Comment 6

[Aus Lacroix [:aus]]

How's the keyboard stuff coming along? Does it look like the only thing we're missing is context menu handling? It'll help me prioritize this item. :)

If we're just missing the context menu i'll open a bug for that work specifically and make it a dependent of this bug.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #6)
> How's the keyboard stuff coming along? Does it look like the only thing
> we're missing is context menu handling? It'll help me prioritize this item.
> :)
> 
> If we're just missing the context menu i'll open a bug for that work
> specifically and make it a dependent of this bug.

Hi,

Please see my comment 3 for other items. I think we still like to have a function to send keyboard event direct to document.activeElement instead of query and send. I am afraid that there may be some race conditions while we use query and send.

And I don't have a way to sendKeys directly to top-level app window because:

1. Since we are in system app, we only have the iframe with null window object which are OOP-ed. So, it's not possible to send key directly to there.
2. If we are testing UI in system app, I don't have how to query window object through Element. Is that possible?

For others, it would not be user-aware. So, it may be fine to skip them.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Ghislain,

If we're just missing the context menu, you don't need to file another bug. Please just use this one and rename it for context menu. Thanks.

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #3)
> (In reply to Ghislain Aus Lacroix [:aus] from comment #2)
> > (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)
> > > 1. to send keyboard event to "active" element
> > 
> > I'm having trouble understanding why having to find the element and then
> > sending the key event would be bad. Integration tests are supposed to have a
> > deterministic path through the UI. If the path is variable but you expect
> > the same outcome, then shouldn't you have a way to test each of those
> > reliably?
> 
> Ghislain,
> 
> Thanks for your suggestion. I had the same idea before but changed with the
> following reasons (please tell me if I am wrong):
> 
> 1. IIRC, some QAs told me that it is more proper to write an integration
> test to simulate user's behavior. If a user uses keyboard, she may not need
> to know which one is focused element, but she may know the top-most UI
> should be closed while she press "OK" or "Back" keys which are default keys
> in TV. To simulate this behavior, we want to send "OK" or "Back" keys to the
> focused element without knowing which one is the active element.
> 
> 2. If there is any events occurred, like a notification shows up, system app
> switches active element to notification. And if this happens between finding
> the element, which should be the active element but not after event got, and
> sending keys to that element, the test case will pass because the element we
> found behaves correctly. But in real case, user still have the UI because
> the key is send to notification but not this UI. 
> 
> 
> The above use case implies two things:
> 
> i. the focus should be correct
> ii. the behavior of keyevent handler should be correct
> 
> We can separate them as two test cases, but these test cases don't like user
> behaviors. We can merge them as one test case, but it needs a way to prevent
> the unexpected change of active element.
> 

Cynthia,

May you confirm that if I am wrong about the above idea since my knowledge of integration test are taught by you??

Comment 10

[Cynthia Tang [:cynthiatang]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #9)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #3)
> > (In reply to Ghislain Aus Lacroix [:aus] from comment #2)
> > > (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)
> > > > 1. to send keyboard event to "active" element
> > > 
> > > I'm having trouble understanding why having to find the element and then
> > > sending the key event would be bad. Integration tests are supposed to have a
> > > deterministic path through the UI. If the path is variable but you expect
> > > the same outcome, then shouldn't you have a way to test each of those
> > > reliably?
> > 
> > Ghislain,
> > 
> > Thanks for your suggestion. I had the same idea before but changed with the
> > following reasons (please tell me if I am wrong):
> > 
> > 1. IIRC, some QAs told me that it is more proper to write an integration
> > test to simulate user's behavior. If a user uses keyboard, she may not need
> > to know which one is focused element, but she may know the top-most UI
> > should be closed while she press "OK" or "Back" keys which are default keys
> > in TV. To simulate this behavior, we want to send "OK" or "Back" keys to the
> > focused element without knowing which one is the active element.
> > 
> > 2. If there is any events occurred, like a notification shows up, system app
> > switches active element to notification. And if this happens between finding
> > the element, which should be the active element but not after event got, and
> > sending keys to that element, the test case will pass because the element we
> > found behaves correctly. But in real case, user still have the UI because
> > the key is send to notification but not this UI. 
> > 
> > 
> > The above use case implies two things:
> > 
> > i. the focus should be correct
> > ii. the behavior of keyevent handler should be correct
> > 
> > We can separate them as two test cases, but these test cases don't like user
> > behaviors. We can merge them as one test case, but it needs a way to prevent
> > the unexpected change of active element.
> > 
> 
> Cynthia,
> 
> May you confirm that if I am wrong about the above idea since my knowledge
> of integration test are taught by you??

Hi John,
I agree with you. We'd like to write the TV UI tests such that user actions are performed in an automated way. Thanks.

Comment 11

[Alberto Pastor [:albertopq]]

Hey Aus!

Who can help to get this done? I've been trying to workaround this by sending click events with button=2 to the element, sending contextmenu events, keyboard events... nothing seems to work. This is important for the TV System app merge effort. Thanks!

Comment 12

[Aus Lacroix [:aus]]

(In reply to Alberto Pastor [:albertopq] from comment #11)
> Hey Aus!
> 
> Who can help to get this done? I've been trying to workaround this by
> sending click events with button=2 to the element, sending contextmenu
> events, keyboard events... nothing seems to work. This is important for the
> TV System app merge effort. Thanks!

Hi Alberto, if this is required by the merge effort I can allocate time to add it to the client now. What's the timeline here? 

"I needed it yesterday" kind of thing or "I need it soon"? :)

Comment 13

[Alberto Pastor [:albertopq]]

Hey Aus! The truth is that we need it as soon as possible, as the integration tests are the priority 1 of the merge at the moment. That said, we still have some cases that don't need contextmenu, so we can continue working on those in the meanwhile. It would be great if we can have it next week, for example. What do you think? Thanks!

Comment 14

[Aus Lacroix [:aus]]

(In reply to Alberto Pastor [:albertopq] from comment #13)
> Hey Aus! The truth is that we need it as soon as possible, as the
> integration tests are the priority 1 of the merge at the moment. That said,
> we still have some cases that don't need contextmenu, so we can continue
> working on those in the meanwhile. It would be great if we can have it next
> week, for example. What do you think? Thanks!

I have a patch ready that adds click and context click support to Marionette.Actions as well as a 'contextMenuClick' method to MarionetteHelper if you don't want to use Actions.

Feel free to have a look. I'll get it committed very soon.

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] nullaus:bug1155116 > mozilla-b2g:master

Comment 16

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/a33cbeeaba357324529a6b3a471bd0e595e83378

Also published marionette-js-client v1.9.5. I do not own marionette-helper right now so I am unable to publish that one, but, you can use it already since you're running everything from a gaia tree.

Comment 17

[Alberto Pastor [:albertopq]]

That sounds great! I'll give it a try! Thanks for fixing this so quick!

Comment 18

[Alberto Pastor [:albertopq]]

Working like a charm :)

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/lib/system.js#L110