801733 - [B2G] Some elements returning incorrect is_displayed properties

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Zac C (:zac)]

Attachment: test case using GaiaTestCase framework

In the two attached test cases I believe HTMLElements are returning incorrect values for their displayed state.

In the first case the target element is the red/yellow loading throbber in the Browser.

in the second case the toolbar is correctly shown as displayed=False initially but not when the image is expanded over it.

Comment 1

[Zac C (:zac)]

Can we bump up the priority of this?

I believe it is causing loads of false positives where B2g is slow to load or failing.

I've never seen an ElementNotVisibleException when interacting with elements which leads me to believe it just thinks everything is visible always.

I also need to rely on it a lot for dynamic waits but at the moment they are pointless.

Comment 3

[David Burns :automatedtester]

So investigating bug 805415 it would appear that we were looking at the wrong field. If I try manipulate phone-number-view[1] with self.marionette.execute_script("document.getElementById('phone-number-view').style.height='40px'");

Then I can change the visible area where the number is. The field that Zac was using does have the same value as phone-number-view but the height is 0 which is why it wasnt getting what we expect.

(Pdb) phone_view.size
{u'width': 66, u'height': 0}

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/index.html#L46

Comment 4

[Zac C (:zac)]

Thanks - I couldn't see it in the raw html.

I've updated the test to suit.

Comment 5

[David Burns :automatedtester]

(In reply to Zac C from comment #0)
> 
> In the first case the target element is the red/yellow loading throbber in
> the Browser.

Looking at this, Marionette is returning the right value. The reason is the element is just empty and is on the page. When you try load a page a new css class called "loading"[1] is added[2]. When the page is finished loading the class is removed but the element is still visible just with nothing to view

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/style/browser.css#L241
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/js/browser.js#L284

Comment 6

[David Burns :automatedtester]

(In reply to Zac C from comment #0)
> in the second case the toolbar is correctly shown as displayed=False
> initially but not when the image is expanded over it.

Currently the selenium project would also show them as visible. We need to start a discussion, which I will do next week at TPAC with number of implementors, to see if this is required.

A work around is to check that the size of the image and the position of the image and make sure that it is overlapping the position and size of the toolbar.

I will update this bug when I have had my discussions

Comment 7

[Zac C (:zac)]

Attachment: call log elements displayed

:automatedtester can you have a look at this test case too? 

I tried briefly to debug with location and size properties but they're not in my build yet.

Comment 8

[David Burns :automatedtester]

Looking into this it looks like we need to get Marionette to understand the viewport. Technically the information that marionette is sending back is correct but from "what the user expects" it is wrong. I will have a look into this more and will probably spin off a new bug for that


(Pdb) all_calls[0].size
{u'width': 480, u'height': 50}

When call log isnt visible
(Pdb) all_calls[0].location
{u'y': 809, u'x': 0}

When call log is visible
(Pdb) all_calls[0].location
{u'y': 109, u'x': 0}

Viewport size
(Pdb) self.marionette.execute_script('return window.innerHeight')
780

Comment 9

[David Burns :automatedtester]

setting flag again

Comment 10

[David Burns :automatedtester]

Speaking to a number of the Selenium developers and this appears to be a unique use case.

I am going to recreate the issue on desktop and then fix it in the selenium project. I am going to create a blocking bug to this bug to have the atoms updated to fix this issue

Comment 11

[David Burns :automatedtester]

tracked this down to the way Firefox OS uses transforms and this is not handled currently in WebDriver and in Marionette. I have a failing test, will fix in Selenium and port across

Comment 12

[David Burns :automatedtester]

Uploaded patch http://codereview.appspot.com/6816090/ and will get a Selenium Core committer to review to make sure I havent made any cross browser bugs

Comment 13

[David Burns :automatedtester]

landed in https://code.google.com/p/selenium/source/detail?r=18085 will now start porting this over for Marionette

Comment 14

[David Burns :automatedtester]

pushed to try https://tbpl.mozilla.org/?tree=Try&rev=7c59547f0180

Comment 15

[David Burns :automatedtester]

pushed to try https://tbpl.mozilla.org/?tree=Try&rev=7c59547f0180

Comment 16

[David Burns :automatedtester]

Attachment: Atoms update

Comment 17

[David Burns :automatedtester]

Attachment: visibility tests

Comment 18

[David Burns :automatedtester]

passed on try. Still waiting for repo sync for b2g to test

Comment 19

[David Burns :automatedtester]

Zac call log test now passes so have set review flags.

Comment 20

[Jonathan Griffin (:jgriffin)]

Comment on attachment 678865 [details] [diff] [review]
Atoms update

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

Hard to verify this so I'm just trusting it's correct.

Comment 21

[Jonathan Griffin (:jgriffin)]

Comment on attachment 678867 [details] [diff] [review]
visibility tests

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

::: testing/marionette/client/marionette/www/cssTransform.html
@@ +11,5 @@
> +  transform: translateX(-10000px);
> +  -webkit-transform: translateX(-10000px);
> +  -o-transform: translateX(-10000px);
> +  -ms-transform: translateX(-10000px);
> +  -moz-transform: translateR(-10000px);

translateR a typo?  I assume this should be translateX

Comment 22

[David Burns :automatedtester]

landed in m-i https://hg.mozilla.org/integration/mozilla-inbound/rev/30899b1b279e
and https://hg.mozilla.org/integration/mozilla-inbound/rev/976225b8f609

Comment 23

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/30899b1b279e
https://hg.mozilla.org/mozilla-central/rev/976225b8f609

Comment 24

[David Burns :automatedtester]

pushed to ash https://hg.mozilla.org/projects/ash/rev/eb790a0a7e44 and https://hg.mozilla.org/projects/ash/rev/663c54106511

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/131db704ce0b
https://hg.mozilla.org/releases/mozilla-aurora/rev/573ad4a1e72e